"osumi.takami...@fujitsu.com" writes:
> [ CREATE_OR_REPLACE_TRIGGER_v18.patch ]
Pushed with some mostly-minor cleanup.
(I know this has been a very long slog. Congratulations for
seeing it through.)
regards, tom lane
Hi,
On Saturday, Nov 7, 2020 2:06 PM Dilip Kumar wrote:
> The patch looks fine to me however I feel that in the test case there are a
> lot
> of duplicate statement which can be reduced e.g.
> +-- 1. Overwrite existing regular trigger with regular trigger
> (without OR REPLACE)
> +create
On Sat, Nov 7, 2020 at 10:00 AM Peter Smith wrote:
>
> Hello Osumi-san.
>
> I have checked the latest v17 patch w.r.t. to my previous comments.
>
> The v17 patch applies cleanly.
>
> make check is successful.
>
> The regenerated docs look OK.
>
> I have no further review comments, so have flagged
Hello Osumi-san.
I have checked the latest v17 patch w.r.t. to my previous comments.
The v17 patch applies cleanly.
make check is successful.
The regenerated docs look OK.
I have no further review comments, so have flagged this v17 as "ready
for committer" -
Hello
On Friday, November 6, 2020 2:25 PM Peter Smith wrote:
> > > Yes, OK. But it might be simpler still just to it like:
> > > "CREATE OR REPLACE TRIGGER works only for replacing a regular (not
> > > constraint) trigger with another regular trigger."
> > Yeah, this kind of supplementary words
Hi,
On Thursday, November 5, 2020 1:22 PM
Peter Smith wrote:
> On Wed, Nov 4, 2020 at 2:53 PM osumi.takami...@fujitsu.com
> wrote:
> > > (1) COMMENT
> > > File: NA
> > > Maybe next time consider using format-patch to make the patch. Then
> > > you can include a comment to give the
Hello Osumi-san.
I have checked the v15 patch with regard to my v14 review comments.
On Wed, Nov 4, 2020 at 2:53 PM osumi.takami...@fujitsu.com
wrote:
> > (1) COMMENT
> > File: NA
> > Maybe next time consider using format-patch to make the patch. Then you
> > can include a comment to give the
Hi
Peter-San, thanks for your support.
On Monday, November 2, 2020 2:39 PM Peter Smith wrote:
> ===
>
> (1) COMMENT
> File: NA
> Maybe next time consider using format-patch to make the patch. Then you
> can include a comment to give the background/motivation for this change.
OK. How about v15 ?
Hello Osumi-san.
Below are my v14 patch review comments for your consideration.
===
(1) COMMENT
File: NA
Maybe next time consider using format-patch to make the patch. Then
you can include a comment to give the background/motivation for this
change.
===
(2) COMMENT
File:
Hi,
From: Tsunakawa, Takayuki < tsunakawa.ta...@fujitsu.com>
> From: osumi.takami...@fujitsu.com
> > > > * I don't think that you've fully thought through the implications
> > > > of replacing a trigger for a table that the current transaction
> > > > has already modified. Is it really
From: osumi.takami...@fujitsu.com
> > > * I don't think that you've fully thought through the implications
> > > of replacing a trigger for a table that the current transaction has
> > > already modified. Is it really sufficient, or even useful, to do
> > > this:
> > >
> > > +/*
> >
Hello
> > * A lesser point is that I think you're overcomplicating the code by
> > applying heap_modify_tuple. You might as well just build the new
> > tuple normally in all cases, and then apply either CatalogTupleInsert or
> CatalogTupleUpdate.
> >
> > * Also, the search for an existing
Hi
I spent too much time to respond to this e-mail. Sorry.
Actually, I got stuck to deal with achieving both
error detection of internal trigger case and pending trigger case.
> * I'm concerned by the fact that there doesn't seem to be any defense against
> somebody replacing a foreign-key
"osumi.takami...@fujitsu.com" writes:
> [ CREATE_OR_REPLACE_TRIGGER_v11.patch ]
I took a quick look through this. I think there's still work left to do.
* I'm concerned by the fact that there doesn't seem to be any defense
against somebody replacing a foreign-key trigger with something that
On Thu, Sep 10, 2020 at 12:34 PM osumi.takami...@fujitsu.com
wrote:
> I attached the v11 patch.
The v11 patch looked OK to me.
Since I have no more review comments I am marking this as "ready for committer".
Kind Regards,
Peter Smith.
Fujitsu Australia
Hello, Peter-San
> > That's a great idea. I've applied this idea to the latest patch v10.
>
>
>
> COMMENT create_trigger.sgml (typo/wording)
>
> "vise versa" -> "vice versa"
Sorry and thank you for all your pointing out.
> BEFORE
> You cannot replace triggers with a different type of
On Wed, Sep 9, 2020 at 11:28 PM osumi.takami...@fujitsu.com
wrote:
> That's a great idea. I've applied this idea to the latest patch v10.
COMMENT create_trigger.sgml (typo/wording)
"vise versa" -> "vice versa"
BEFORE
You cannot replace triggers with a different type of trigger, that
Hi
> Those are fixed OK now, but I found 2 new review points.
>
>
>
> COMMENT trigger.c (typo)
>
> + ereport(ERROR,
> + (errcode(ERRCODE_DUPLICATE_OBJECT),
> + errmsg("trigger \"%s\" for relation \"%s\" is a constraint trigger",
> + stmt->trigname, RelationGetRelationName(rel)),
> +
On Tue, Sep 8, 2020 at 9:36 PM osumi.takami...@fujitsu.com
wrote:
>
> I've fixed all except one point.
Thanks for addressing my previous review comments in your new v09 patch.
Those are fixed OK now, but I found 2 new review points.
COMMENT trigger.c (typo)
+ ereport(ERROR,
+
Hi, Peter-San
I've fixed all except one point.
> My only remaining comments are for trivial stuff:
Not trivial but important.
> COMMENT trigger.c (tab alignment)
>
> @@ -184,6 +185,13 @@ CreateTrigger(CreateTrigStmt *stmt, const char
> *queryString,
> char*oldtablename = NULL;
> char
Hi Osumi-san.
Thanks for addressing my previous review comments in your new v08 patch.
I have checked it again.
My only remaining comments are for trivial stuff:
COMMENT trigger.c (tab alignment)
@@ -184,6 +185,13 @@ CreateTrigger(CreateTrigStmt *stmt, const char
*queryString,
char
Hi, Peter
You gave me two posts for my patch review.
Thank you so much. I'll write all my replies into this post.
>
>
> COMMENT pg_constraint.c (wrong comment?)
>
> - * The new constraint's OID is returned.
> + * The new constraint's OID is returned. (This will be the same as
> + *
On Mon, Aug 24, 2020 at 9:33 PM osumi.takami...@fujitsu.com
wrote:
> I've fixed my v06 and created v07.
Hi Osumi-san.
I have reviewed the test code of the v07 patch.
Below are my comments.
COMMENT (confusing functions)
+create function before_replacement() returns trigger as $$
+begin
On Mon, Aug 24, 2020 at 9:33 PM osumi.takami...@fujitsu.com
wrote:
> I've fixed my v06 and created v07.
Hi Osumi-san.
I have reviewed the source code of the v07 patch.
(I also reviewed the test cases but I will share those comments as a
separate post).
Below are my comments - sorry, many of
Hi,
Thanks, Peter.
> FYI - The latest patch (v06) has conflicts when applied.
I've fixed my v06 and created v07.
Also, I added one test to throw an error
to avoid a situation that trigger which has pending events are replaced by
any other trigger in the same session.
Best,
Takamichi
On Thu, Aug 20, 2020 at 12:11 PM osumi.takami...@fujitsu.com
wrote:
> I have fixed my patch again.
Hi Osumi-san,
FYI - The latest patch (v06) has conflicts when applied.
Kind Regards,
Peter Smith.
Fujitsu Australia
osumi.takami...@fujitsu.com:
* I'm a little bit concerned about the semantics of changing the
tgdeferrable/tginitdeferred properties of an existing trigger. If there are
trigger
events pending, and the trigger is redefined in such a way that those events
should already have been fired, what
Dear Tom Lane
Thanks for your so many fruitful comments !
I have fixed my patch again.
On the other hand, there're some questions left
that I'd like to discuss.
> * You missed updating equalfuncs.c/copyfuncs.c. Pretty much any change in a
> Node struct will require touching backend/nodes/
"osumi.takami...@fujitsu.com" writes:
> Also, I'm waiting for other kind of feedbacks from anyone.
As David pointed out, this needs to be rebased, though it looks like
the conflict is pretty trivial.
A few other notes from a quick look:
* You missed updating equalfuncs.c/copyfuncs.c. Pretty
On 12/1/19 8:56 PM, osumi.takami...@fujitsu.com wrote:
The latest patch includes calls to heap_open(), causing its compilation to fail.
Could you please send a rebased version of the patch? I have moved the entry to
next CF, waiting on author.
Thanks. I've fixed where you pointed out.
This
Dear Michael san
> The latest patch includes calls to heap_open(), causing its compilation to
> fail.
> Could you please send a rebased version of the patch? I have moved the entry
> to
> next CF, waiting on author.
Thanks. I've fixed where you pointed out.
Also, I'm waiting for other kind of
On Thu, Aug 01, 2019 at 09:49:53PM +1200, Thomas Munro wrote:
> The end of CF1 is here. I've moved this patch to CF2 (September) in
> the Commitfest app. Of course, everyone is free to continue
> discussing the patch before then. When you have a new version, please
> set the status to "Needs
Dear Tom Lane
Thank you so much for your comment.
> * Upthread you asked about changing the lock level to be AccessExclusiveLock
> if
> the trigger already exists, but the patch doesn't actually do that. Which is
> fine by
> me, because that sounds like a perfectly bad idea.
Why I suggested
On 2019-Jul-22, osumi.takami...@fujitsu.com wrote:
> Dear Surafel
>
> Thank you for your check of my patch.
> I’ve made the version 03 to
> fix what you mentioned about my patch.
>
> I corrected my wrong bracing styles and
> also reduced the amount of my regression test.
> Off course, I erased
On Wed, Jul 31, 2019 at 1:33 PM Michael Paquier wrote:
> On Tue, Jul 30, 2019 at 04:44:11PM -0400, Tom Lane wrote:
> > We do not test \h output in any existing regression test, and we're
> > not going to start doing so in this one. For one thing, the expected
> > URL would break every time we
On Tue, Jul 30, 2019 at 04:44:11PM -0400, Tom Lane wrote:
> We do not test \h output in any existing regression test, and we're
> not going to start doing so in this one. For one thing, the expected
> URL would break every time we forked off a new release branch.
> (There would surely be value in
"osumi.takami...@fujitsu.com" writes:
> [ CREATE_OR_REPLACE_TRIGGER_v03.patch ]
I took a quick look through this just to see what was going on.
A few comments:
* Upthread you asked about changing the lock level to be
AccessExclusiveLock if the trigger already exists, but the patch doesn't
Dear Surafel
Thank you for your check of my patch.
I’ve made the version 03 to
fix what you mentioned about my patch.
I corrected my wrong bracing styles and
also reduced the amount of my regression test.
Off course, I erased unnecessary
white spaces and the C++ style comment.
Regards,
Hi Takamichi Osumi,
On Tue, Jul 9, 2019
> I've rebased the previous patch to be applied
>
I don't test your patch fully yet but here are same comment.
There are same white space issue like here
- bool is_internal)
+ bool is_internal,
+ Oid existing_constraint_oid)
in a few place
+ // trigoid
Dear Michael san
> > So, I'll fix this within a couple of weeks.
> Please note that I have switched the patch as waiting on author.
Thanks for your support.
I've rebased the previous patch to be applied
to the latest PostgreSQL without any failure of regression tests.
Best,
Takamichi
On Wed, Jul 03, 2019 at 04:37:00AM +, osumi.takami...@fujitsu.com wrote:
> I really appreciate your comments.
> Recently, I'm very busy because of my work.
> So, I'll fix this within a couple of weeks.
Please note that I have switched the patch as waiting on author.
--
Michael
signature.asc
Dear Mr. Thomas
> The July Commitfest is now beginning. To give the patch the best chance of
> attracting reviewers, could you please post a rebased version? The last
> version
> doesn't apply.
I really appreciate your comments.
Recently, I'm very busy because of my work.
So, I'll fix this
On Tue, Mar 5, 2019 at 10:19 PM David Steele wrote:
> On 2/28/19 10:43 AM, Osumi, Takamichi wrote:
> > One past thread about introducing CREATE OR REPLACE TRIGGER into the syntax
> >
> > had stopped without complete discussion in terms of LOCK level.
> >
> > The past thread is this. I'd like to
On 2/28/19 10:43 AM, Osumi, Takamichi wrote:
One past thread about introducing CREATE OR REPLACE TRIGGER into the syntax
had stopped without complete discussion in terms of LOCK level.
The past thread is this. I'd like to inherit this one.
Since this patch landed at the last moment in the
o register my patch !
Please withdraw the old one, "extension patch to add OR REPLACE clause to
CREATE TRIGGER".
My latest version is "extension patch of CREATE OR REPLACE TRIGGER".
Thanks !
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On Thu, 28 Feb 2019 at 21:44, Osumi, Takamichi
wrote:
> I've made a patch to add CREATE OR REPLACE TRIGGER with some basic tests in
> triggers.sql.
Hi,
I see there are two patch entries in the commitfest for this. Is that
a mistake? If so can you "Withdraw" one of them?
--
David Rowley
Dear hackers
Hi there !
One past thread about introducing CREATE OR REPLACE TRIGGER into the syntax
had stopped without complete discussion in terms of LOCK level.
The past thread is this. I'd like to inherit this one.
47 matches
Mail list logo
