From: | Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> |
---|---|
To: | Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> |
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:36:09 |
Message-ID: | CAEZATCWGGmigGBzLHkJm5Ccv2mMxXmwi3+uq0yhwDHm-tsvSLg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tue, 30 Jan 2024 at 11:58, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> This looks quite nice, thanks. LGTM.
>
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, so I've added a new test
there.)
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.
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.
Regards,
Dean
Attachment | Content-Type | Size |
---|---|---|
support-merge-into-view-v13.patch | text/x-patch | 132.2 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | a.imamov | 2024-02-29 09:43:25 | Re: Potential issue in ecpg-informix decimal converting functions |
Previous Message | Andres Freund | 2024-02-29 09:35:31 | Re: Remove AIX Support (was: Re: Relation bulk write facility) |