Command Triggers

Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:

As proposed by Jan Wieck on hackers and discussed in Ottawa at the
Clusters Hackers Meeting, the most (only?) workable way to ever have DDL
triggers is to have "command triggers" instead.

[...]

The v1 patch attached contains implementation for some commands only.

Here's a v2 which is only about cleaning up the merge from master. The
recent DROP rework conflicted badly with the command triggers patch,
quite obviously. I didn't reimplement DROP COMMAND TRIGGER in terms of
the new facilities yet, though.

So, any comment on the approach before I complete the rewriting of the
commands, the out/read funcs, and add more commands to the trigger
support code?

https://github.com/dimitri/postgres/compare/master...command_triggers

FWIW (scheduling mainly), I don't think I'm going to spend more time on
this work until I get some comments, because all that remains to be done
is about building on top of what I've already been doing here.

Well, I'd like to implement command triggers on replica too, but baring
psql support and a bunch of commands not there yet, that's about it as
far as features go.

Oh, and have expressions rewriting from the parsetree (default, check)
actually work (rather than segfault) is still a TODO too.

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

Hi Dimitri, Hi all,

On Tuesday, November 08, 2011 06:47:13 PM Dimitri Fontaine wrote:

As proposed by Jan Wieck on hackers and discussed in Ottawa at the
Clusters Hackers Meeting, the most (only?) workable way to ever have DDL
triggers is to have "command triggers" instead.
Rather than talking about catalogs and the like, it's all about firing a
registered user-defined function before or after processing the utility
command, or instead of processing it. This naturally involves a new
catalog (pg_cmdtrigger) and some new subcommands of CREATE and DROP
TRIGGER, and some support code for calling the function at the right
time.

Great ;)

fwiw I think thats the way forward as well.

The patch obviously isn't thought to be commitable yet, so I am not going to
do a very detailed code level review. Attached is a patch with low level
targeted comments and such I noticed while reading it.

At this state the important things are highlevel so I will try to concentrate
on those:

== the trigger function ==
I don't like the set of options parsed to the trigger functions very much. To
cite an example of yours those currently are:
* cmd_string text
* cmd_nodestring text
* schemaname text
* relname text

I think at least a
* command_tag text
is missing. Sure, you can disambiguate it by creating a function for every
command type but that seems pointlessly complex for many applications. And
adding it should be trivial as its already tracked ;)

Currently the examples refer to relname as relname which I dislike as that
seems to be too restricted to one use case. The code is already doing it
correctly (as objectname) though ;)

Why is there explicit documentation of not checking the arguments of said
trigger function? That seems to be a bit strange to me.

schemaname currently is mightily unusable because whether its sent or not
depends wether the table was created with a schemaname specified or not. That
doesn't seem to be a good approach.

That brings me to the next point:

== normalization of statements ==
Currently the normalization is a bit confusing. E.g. for:

CREATE SCHEMA barf;
SET search_path = barf;
CREATE TYPE bart AS ENUM ('a', 'b');
CREATE TABLE bar(a int, b bigint, c serial, d text, e varchar, "F" text, g
bart, h int4);

one gets

CREATE TABLE bar (a pg_catalog.int4,b pg_catalog.int8,c serial,d text,e
pg_catalog.varchar,F text,g bart,h int4);

Which points out two problems:
* inconsistent schema prefixing
* no quoting

Imo the output should fully schema qualify anything including operators. Yes,
thats rather ugly but anything else seems to be too likely to lead to
problems.

As an alternative it would be possible to pass the current search path but
that doesn't seem to the best approach to me;

Currently CHECK() constraints are not decodable, but inside those the same
quoting/qualifying rules should apply.

Then there is nice stuff like CREATE TABLE .... (LIKE ...);
What shall that return in future? I vote for showing it as the plain CREATE
TABLE where all columns are specified.

That touches another related topic. Dim's work in adding support for utility
cmd support in nodeToString and friends possibly will make the code somewhat
command trigger specific. Is there any other usage we envision?

== grammar ==

* multiple actions:
I think it would sensible to allow multiple actions on which to trigger to be
specified just as you can for normal triggers. I also would like an ALL option

* options:
Currently the grammar allows options to be passed to command triggers. Do we
want to keep that? If yes, with the same arcane set of datatypes as in data
triggers?
If yes it needs to be wired up.

* schema qualification:
An option to add triggers only to a specific schema would be rather neat for
many scenarios.
I am not totally sure if its worth the hassle of defining what happens in the
edge cases of e.g. moving from one into another schema though.

== locking ==

Currently there is no locking strategy at all. Which e.g. means that there is
no protection against two sessions changing stuff concurrently or such.

Was that just left out - which is ok - or did you miss that?

I think it would be ok to just always grab row level locks there.

== permissions ==

Command triggers should only be allowed for the database owner.

== recursion ==
I contrast to data triggers command triggers cannot change what is done. They
can either prevent it from running or not. Which imo is fine.
The question I have is whether it would be a good idea to make it easy to
prevent recursion.... Especially if the triggers wont be per schema.

== calling the trigger ==

Imo the current callsite in ProcessUtility isn't that good. I think it would
make more sense moving it into standard_ProcessUtility. It should be *after*
the check_xact_readonly check in there in my opinion.

I am also don't think its a good idea to run the
ExecBeforeOrInsteadOfCommandTriggers stuff there because thats allso the path
that COMMIT/BEGIN and other stuff take. Those are pretty "fast path".

I suggest making two switches in standard_ProcessUtility, one for the non-
command trigger stuff which returns on success and one after that. Only after
the first triggers are checked.

I wonder whether its the right choice to run triggers on untransformed input.
I.e. CREATE TABLE ... (LIKE ...) seems to only make sense to me after
transforming.

Also youre very repeatedly transforming the parstree into a string. It would
be better doing that only once instead of every trigger...

Ok, thats the first round of high level stuff...

Cool Work!

Some lower level annotations:

* many functions should be static but are not. Especially in cmdtrigger.c
* Either tgenable's datatype or its readfunc is wrong (watch the compiler ;))
* ruleutils.c:
* generic routine for IF EXISTS, CASCADE, ...

Greetings,

Andres

Hi,

On Tuesday, November 08, 2011 06:47:13 PM Dimitri Fontaine wrote:

exception to that rule would be SELECT INTO and CREATE TABLE AS SELECT,
and my proposal would be to add specific call sites to the functions
I've provided in the attached patch rather than try to contort them into
being a good citizen as a utility command.

I would very much prefer to make them utility commands and rip out the
executor support for that.
It doesn't look that complicated and would allow us to get rid of the
partially duplicated defineRelation and related stuff from the executor where
its violating my aestetic feelings ;)

Is there something making that especially hard that I overlooked? The
infrastructure for doing so seems to be there.

Andres

Andres Freund <andres@anarazel.de> writes:

On Tuesday, November 08, 2011 06:47:13 PM Dimitri Fontaine wrote:

exception to that rule would be SELECT INTO and CREATE TABLE AS SELECT,
and my proposal would be to add specific call sites to the functions
I've provided in the attached patch rather than try to contort them into
being a good citizen as a utility command.

I would very much prefer to make them utility commands and rip out the
executor support for that.
Is there something making that especially hard that I overlooked? The
infrastructure for doing so seems to be there.

Well, I think the main problem is going to be shunting the query down
the right parsing track (SELECT versus utility-statement) early enough.
Making this work cleanly would be a bigger deal than I think you're
thinking.

But yeah, it would be nicer if the executor didn't have to worry about
this. +1 if you're willing to take on the work.

regards, tom lane

On Thursday, December 01, 2011 07:21:25 PM Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On Tuesday, November 08, 2011 06:47:13 PM Dimitri Fontaine wrote:

exception to that rule would be SELECT INTO and CREATE TABLE AS SELECT,
and my proposal would be to add specific call sites to the functions
I've provided in the attached patch rather than try to contort them into
being a good citizen as a utility command.

I would very much prefer to make them utility commands and rip out the
executor support for that.
Is there something making that especially hard that I overlooked? The
infrastructure for doing so seems to be there.

Well, I think the main problem is going to be shunting the query down
the right parsing track (SELECT versus utility-statement) early enough.
Making this work cleanly would be a bigger deal than I think you're
thinking.

Yes. I forgot how ugly SELECT INTO is...

But yeah, it would be nicer if the executor didn't have to worry about
this. +1 if you're willing to take on the work.

I won't promise anything but I will play around with the grammar and see if I
find a nice enough way.

Andres

On Thursday, December 01, 2011 07:21:25 PM Tom Lane wrote:

Well, I think the main problem is going to be shunting the query down
the right parsing track (SELECT versus utility-statement) early enough.
Making this work cleanly would be a bigger deal than I think you're
thinking.

Obviously that depends on the definition of clean...

Changing the grammar to make that explicit seems to involve a bit too many
changes on a first glance. The cheap way out seems to be to make the decision
in analyze.c:transformQuery.
Would that be an acceptable way forward?

Andres

On Sat, Nov 26, 2011 at 7:20 PM, Dimitri Fontaine
<dimitri@2ndquadrant.fr> wrote:

FWIW (scheduling mainly), I don't think I'm going to spend more time on
this work until I get some comments, because all that remains to be done
is about building on top of what I've already been doing here.

+1 overall

One of the main use cases for me is the ability to
* prevent all commands
* prevent extension commands - to maintain stable execution environment

Those are especially important because in 9.2 DDL commands will cause
additional locking overheads, so preventing DDL will be essential to
keeping performance stable in high txn rate databases.

So I'd like to see a few more triggers that work out of the box for
those cases (perhaps wrapped by a function?). It would also allow a
more useful man page example of how to use this.

On the patch, header comment for cmdtrigger.c needs updating.
Comments for DropCmdTrigger etc look like you forgot to update them
after cut and paste.

Comments say "For any given command tag, you can have either Before
and After triggers, or
Instead Of triggers, not both.", which seems like a great thing to
put in the docs.

Looks good.

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

Andres Freund <andres@anarazel.de> writes:

On Thursday, December 01, 2011 07:21:25 PM Tom Lane wrote:

Making this work cleanly would be a bigger deal than I think you're
thinking.

Obviously that depends on the definition of clean...

Changing the grammar to make that explicit seems to involve a bit too many
changes on a first glance. The cheap way out seems to be to make the decision
in analyze.c:transformQuery.
Would that be an acceptable way forward?

ITYM transformStmt, but yeah, somewhere around there is probably reasonable.
The way I'm imagining this would work is that IntoClause disappears from
Query altogether: analyze.c would build a utility statement
CreateTableAs, pull the IntoClause out of the SelectStmt structure and
put it into the utility statement, and then proceed much as we do for
ExplainStmt (and for the same reasons).

regards, tom lane

Hi,

Hm. I just noticed a relatively big hole in the patch: The handling of
deletion of dependent objects currently is nonexistant because they don't go
through ProcessUtility...

Currently thinking what the best way to nicely implement that is. For other
commands the original command string is passed - that obviously won't work for
that case...

Andres

Hi all,

There is also the point about how permission checks on the actual commands (in
comparison of modifying command triggers) and such are handled:

BEFORE and INSTEAD will currently be called independently of the fact whether
the user is actually allowed to do said action (which is inconsistent with
data triggers) and indepentent of whether the object they concern exists.

I wonder if anybody considers that a problem?

Andres

Hi,

First thing first: thank you Andres for a great review, I do appreciate
it. Please find attached a newer version of the patch. The github
repository is also updated.

Andres Freund <andres@anarazel.de> writes:

I think at least a
* command_tag text

Added.

Why is there explicit documentation of not checking the arguments of said
trigger function? That seems to be a bit strange to me.

The code is searching for a function with the given name and 5 text
arguments, whatever you say in the command. That could be changed later
on if there's a need.

schemaname currently is mightily unusable because whether its sent or not
depends wether the table was created with a schemaname specified or not. That
doesn't seem to be a good approach.

I'm not sure about that, we're spiting out what the user entered.

Imo the output should fully schema qualify anything including operators. Yes,
thats rather ugly but anything else seems to be too likely to lead to
problems.

Will look into qualifying names.

As an alternative it would be possible to pass the current search path but
that doesn't seem to the best approach to me;

The trigger runs with the same search_path settings as the command, right?

Then there is nice stuff like CREATE TABLE .... (LIKE ...);
What shall that return in future? I vote for showing it as the plain CREATE
TABLE where all columns are specified.

I don't think so. Think about the main use cases for this patch,
auditing and replication. Both cases you want to deal with what the
user said, not what the system understood.

I think it would sensible to allow multiple actions on which to trigger to be
specified just as you can for normal triggers. I also would like an ALL option

Both are now implemented.

When dealing with an ANY command trigger, your procedure can get fired
on a command for which we don't have internal support for back parsing
etc, so you only get the command tag not null. I think that's the way to
go here, as I don't want to think we need to implement full support for
command triggers on ALTER OPERATOR FAMILY from the get go.

Currently the grammar allows options to be passed to command triggers. Do we
want to keep that? If yes, with the same arcane set of datatypes as in data
triggers?
If yes it needs to be wired up.

In fact it's not the case, you always get called with the same 5
arguments, all nullable now that we have ANY COMMAND support.

* schema qualification:
An option to add triggers only to a specific schema would be rather neat for
many scenarios.
I am not totally sure if its worth the hassle of defining what happens in the
edge cases of e.g. moving from one into another schema though.

I had written down support for a WHEN clause in command triggers, but
the exact design has yet to be worked out. I'd like to have this
facility but I'd like it even more if that could happen in a later
patch.

Currently there is no locking strategy at all. Which e.g. means that there is
no protection against two sessions changing stuff concurrently or such.

Was that just left out - which is ok - or did you miss that?

I left out the locking as of now. I tried to fix some of it in the new
patch though, but I will need to spend more time on that down the road.

Command triggers should only be allowed for the database owner.

Yes, that needs to happen soon, along with pg_dump and psql support.

I contrast to data triggers command triggers cannot change what is done. They
can either prevent it from running or not. Which imo is fine.
The question I have is whether it would be a good idea to make it easy to
prevent recursion.... Especially if the triggers wont be per schema.

You already have

ATER TRIGGER foo ON COMMAND create table DISABLE;

that you can use from the trigger procedure itself. Of course, locking
is an issue if you want to go parallel on running commands with
recursive triggers attached. I think we might be able to skip solving
that problem in the first run.

Imo the current callsite in ProcessUtility isn't that good. I think it would
make more sense moving it into standard_ProcessUtility. It should be *after*
the check_xact_readonly check in there in my opinion.

Makes sense, will fix in the next revision.

I am also don't think its a good idea to run the
ExecBeforeOrInsteadOfCommandTriggers stuff there because thats allso the path
that COMMIT/BEGIN and other stuff take. Those are pretty "fast path".

I suggest making two switches in standard_ProcessUtility, one for the non-
command trigger stuff which returns on success and one after that. Only after
the first triggers are checked.

Agreed.

Also youre very repeatedly transforming the parstree into a string. It would
be better doing that only once instead of every trigger...

It was only done once per before and instead of triggers (you can't have
both on the same command), and another time for any and all after
triggers, meaning at most twice. It's now down to only once, at most.

* many functions should be static but are not. Especially in cmdtrigger.c

Fixed.

* Either tgenable's datatype or its readfunc is wrong (watch the compiler ;))
* ruleutils.c:
* generic routine for IF EXISTS, CASCADE, ...

Will have to wait until next revision.

Thanks you again for a great review, regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

Excerpts from Andres Freund's message of vie dic 02 19:09:47 -0300 2011:

Hi all,

There is also the point about how permission checks on the actual commands (in
comparison of modifying command triggers) and such are handled:

BEFORE and INSTEAD will currently be called independently of the fact whether
the user is actually allowed to do said action (which is inconsistent with
data triggers) and indepentent of whether the object they concern exists.

I wonder if anybody considers that a problem?

Hmm, we currently even have a patch (or is it already committed?) to
avoid locking objects before we know the user has permission on the
object. Getting to the point of calling the trigger would surely be
even worse.

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Fri, Dec 2, 2011 at 7:09 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:

Hmm, we currently even have a patch (or is it already committed?) to
avoid locking objects before we know the user has permission on the
object.  Getting to the point of calling the trigger would surely be
even worse.

I committed a first round of cleanup in that area, but unfortunately
there is a lot more to be done.

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

On Saturday, December 03, 2011 01:09:48 AM Alvaro Herrera wrote:

Excerpts from Andres Freund's message of vie dic 02 19:09:47 -0300 2011:

Hi all,

There is also the point about how permission checks on the actual
commands (in comparison of modifying command triggers) and such are
handled:

BEFORE and INSTEAD will currently be called independently of the fact
whether the user is actually allowed to do said action (which is
inconsistent with data triggers) and indepentent of whether the object
they concern exists.

I wonder if anybody considers that a problem?

Hmm, we currently even have a patch (or is it already committed?) to
avoid locking objects before we know the user has permission on the
object. Getting to the point of calling the trigger would surely be
even worse.

Well, calling the trigger won't allow them to lock the object. It doesn't even
confirm the existance of the table.

Andres

Simon Riggs <simon@2ndQuadrant.com> writes:

Those are especially important because in 9.2 DDL commands will cause
additional locking overheads, so preventing DDL will be essential to
keeping performance stable in high txn rate databases.

The patch now implements "any command" triggers, and you have the
command tag to branch in the function if you need to.

CREATE TRIGGER noddl
INSTEAD OF ANY COMMAND
EXECUTE PROCEDURE cancel_any_ddl();

So I'd like to see a few more triggers that work out of the box for
those cases (perhaps wrapped by a function?). It would also allow a
more useful man page example of how to use this.

We could solve that by providing an extension implementing command
triggers ready for use. One that allows to easily switch on and off the
capability of running commands seems like a good candidate.

That said, with the current coding, you can't have both a instead of
trigger on "any command" and a before or after trigger on any given
command, so installing that extension would prevent you from any other
usage of command triggers.

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

On Saturday, December 03, 2011 12:04:57 AM Dimitri Fontaine wrote:

Hi,

First thing first: thank you Andres for a great review, I do appreciate
it. Please find attached a newer version of the patch. The github
repository is also updated.

Andres Freund <andres@anarazel.de> writes:

I think at least a
* command_tag text

Added.

Why is there explicit documentation of not checking the arguments of said
trigger function? That seems to be a bit strange to me.

The code is searching for a function with the given name and 5 text
arguments, whatever you say in the command. That could be changed later
on if there's a need.

Forget it, i was too dumb to remember the code when I wrote the above ;)

schemaname currently is mightily unusable because whether its sent or not
depends wether the table was created with a schemaname specified or not.
That doesn't seem to be a good approach.

I'm not sure about that, we're spiting out what the user entered.

No, were not. Were writing out something that semantically the same what the
user entered. At several places NULL/NOT NULL and such get added.

As an alternative it would be possible to pass the current search path
but that doesn't seem to the best approach to me;

The trigger runs with the same search_path settings as the command, right?

For many things it makes sense to specify the search path of a function uppon
creation of the function. Even moreso if you have security definer
functions...
Otherwise you can get into troubles via operators and such...

Then there is nice stuff like CREATE TABLE .... (LIKE ...);
What shall that return in future? I vote for showing it as the plain
CREATE TABLE where all columns are specified.

I don't think so. Think about the main use cases for this patch,
auditing and replication. Both cases you want to deal with what the
user said, not what the system understood.

Is that so? In the replication case the origin table very well may look
differently on the master compared to the standby. Which is about impossible
to diagnose with the above behaviour.

I think it would sensible to allow multiple actions on which to trigger
to be specified just as you can for normal triggers. I also would like
an ALL option

Both are now implemented.

Cool, will check.

When dealing with an ANY command trigger, your procedure can get fired
on a command for which we don't have internal support for back parsing
etc, so you only get the command tag not null. I think that's the way to
go here, as I don't want to think we need to implement full support for
command triggers on ALTER OPERATOR FAMILY from the get go.

Thats ok with me.

Currently the grammar allows options to be passed to command triggers. Do
we want to keep that? If yes, with the same arcane set of datatypes as
in data triggers?
If yes it needs to be wired up.

In fact it's not the case, you always get called with the same 5
arguments, all nullable now that we have ANY COMMAND support.

I think you misunderstood me. Check the following excerpt from gram.y:
CreateCmdTrigStmt:
CREATE TRIGGER name TriggerActionTime COMMAND trigger_command_list
EXECUTE PROCEDURE func_name '(' TriggerFuncArgs ')'

You have TriggerfuncArgs there but you ignore them.

* schema qualification:
An option to add triggers only to a specific schema would be rather neat
for many scenarios.
I am not totally sure if its worth the hassle of defining what happens in
the edge cases of e.g. moving from one into another schema though.

I had written down support for a WHEN clause in command triggers, but
the exact design has yet to be worked out. I'd like to have this
facility but I'd like it even more if that could happen in a later
patch.

Hm. I am not sure a generic WHEN clause is needed. In my opinion schema
qualification would be enough...
A later patch is fine imo.

I contrast to data triggers command triggers cannot change what is done.
They can either prevent it from running or not. Which imo is fine.
The question I have is whether it would be a good idea to make it easy to
prevent recursion.... Especially if the triggers wont be per schema.

You already have

ATER TRIGGER foo ON COMMAND create table DISABLE;

that you can use from the trigger procedure itself. Of course, locking
is an issue if you want to go parallel on running commands with
recursive triggers attached. I think we might be able to skip solving
that problem in the first run.

hm. Not yet convinced. I need to think about it a bit.

Have fun ;)

Andres

On Friday, December 02, 2011 03:09:55 AM Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On Thursday, December 01, 2011 07:21:25 PM Tom Lane wrote:

Making this work cleanly would be a bigger deal than I think you're
thinking.

Obviously that depends on the definition of clean...

Changing the grammar to make that explicit seems to involve a bit too
many changes on a first glance. The cheap way out seems to be to make
the decision in analyze.c:transformQuery.
Would that be an acceptable way forward?

ITYM transformStmt, but yeah, somewhere around there is probably
reasonable. The way I'm imagining this would work is that IntoClause
disappears from Query altogether: analyze.c would build a utility
statement
CreateTableAs, pull the IntoClause out of the SelectStmt structure and
put it into the utility statement, and then proceed much as we do for
ExplainStmt (and for the same reasons).

Ok. Because my book turned out to be boring I started looking at this. I
wonder if wouldn't be better to do check in directly in raw_parser(). The
advantage would be that that magically would fix the issue of not logging
CTAS/select when using log_statement = ddl and we don't need a snapshot or
such so that shouldn't be a problem.

Any arguments against doing so?

Andres

On Friday, December 02, 2011 03:09:55 AM Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On Thursday, December 01, 2011 07:21:25 PM Tom Lane wrote:

Making this work cleanly would be a bigger deal than I think you're
thinking.

Obviously that depends on the definition of clean...

Changing the grammar to make that explicit seems to involve a bit too
many changes on a first glance. The cheap way out seems to be to make
the decision in analyze.c:transformQuery.
Would that be an acceptable way forward?

ITYM transformStmt, but yeah, somewhere around there is probably
reasonable. The way I'm imagining this would work is that IntoClause
disappears from Query altogether: analyze.c would build a utility
statement
CreateTableAs, pull the IntoClause out of the SelectStmt structure and
put it into the utility statement, and then proceed much as we do for
ExplainStmt (and for the same reasons).

I have a patch which basically does all this minus some cleanup. I currently
do the the differentiation for SELECT INTO (not CREATE AS, thats done in
gram.y) in raw_parser but thats quick to move if others don't aggree on that.

I have two questions now:

First, does anybody think it would be worth getting rid of the duplication
from OpenIntoRel (formerly from execMain.c) in regard to DefineRelation()?
I noticed that there already is some diversion between both. E.g. CREATE TABLE
frak TABLESPACE pg_global AS SELECT 1; is possible while it would be forbidden
via a plain CREATE TABLE. (I will send a fix for this separately).

Secondly, I am currently wondering whether it would be a good idea to use the
ModifyTable infrastructure for doing the insertion instead an own DestReceiver
infrastructure thats only used for CTAS.
It looks a bit too complicated to do this without removing the bulk insert and
HEAP_INSERT_SKIP_WAL optimizations.

Andres

Andres Freund <andres@anarazel.de> writes:

I have two questions now:

First, does anybody think it would be worth getting rid of the duplication
from OpenIntoRel (formerly from execMain.c) in regard to DefineRelation()?

That's probably reasonable to do, since as you say it would remove the
opportunity for bugs-of-omission in the CTAS table creation step.
OTOH, if you find yourself having to make any significant changes to
DefineRelation, then maybe not.

Secondly, I am currently wondering whether it would be a good idea to use the
ModifyTable infrastructure for doing the insertion instead an own DestReceiver
infrastructure thats only used for CTAS.

I think this is probably a bad idea; it will complicate matters and buy
little. There's not a lot of stuff needed for the actual data insertion
step, since we know the table can't have any defaults, constraints,
triggers, etc as yet.

regards, tom lane