From: | Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> |
---|---|
To: | Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> |
Cc: | vignesh C <vignesh21(at)gmail(dot)com>, jian he <jian(dot)universality(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: Supporting MERGE on updatable views |
Date: | 2024-02-29 09:50:05 |
Message-ID: | 202402290950.dgihici7owpx@alvherre.pgsql |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 2024-Feb-29, Dean Rasheed wrote:
> Going over this again, I spotted a bug -- the UPDATE path in
> ExecMergeMatched() wasn't calling ExecUpdateEpilogue() for
> trigger-updatable views, because it wasn't setting updateCxt.updated
> to true. (This matters if you have an auto-updatable view WITH CHECK
> OPTION on top of a trigger-updatable view,
Oh, right ... glad you found that. It sounds a bit convoluted and it
would have been a pain if users had found out afterwards.
> [...], so I've added a new test there.)
Great, thanks.
> Rather than setting updateCxt.updated to true, making the
> trigger-invoking code in ExecMergeMatched() diverge from ExecUpdate(),
> a better fix is to simply remove the UpdateContext.updated flag
> entirely. The only place that reads it is this code in
> ExecMergeMatched():
>
> if (result == TM_Ok && updateCxt.updated)
> {
> ExecUpdateEpilogue(context, &updateCxt, resultRelInfo,
> tupleid, NULL, newslot);
>
> where result is the result from ExecUpdateAct(). However, all paths
> through ExecUpdateAct() that return TM_Ok also set updateCxt.updated
> to true, so the flag is redundant. It looks like that has always been
> the case, ever since it was introduced. Getting rid of it is a useful
> simplification, and brings the UPDATE path in ExecMergeMatched() more
> into line with ExecUpdate(), which always calls ExecUpdateEpilogue()
> if ExecUpdateAct() returns TM_Ok.
This is a great find! I agree with getting rid of
UpdateContext->updated as a separate commit.
> Aside from that, I've done a little more copy-editing and added a few
> more test cases, and I think this is pretty-much good to go, though I
> think I'll split this up into separate commits, since removing
> UpdateContext.updated isn't really related to the MERGE INTO view
> feature.
By all means let's get the feature out there. It's not a frequently
requested thing but it does seem to come up.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Tiene valor aquel que admite que es un cobarde" (Fernandel)
From | Date | Subject | |
---|---|---|---|
Next Message | Amit Kapila | 2024-02-29 09:53:30 | Re: Synchronizing slots from primary to standby |
Previous Message | a.imamov | 2024-02-29 09:43:25 | Re: Potential issue in ecpg-informix decimal converting functions |