Thread: Supporting MERGE on updatable views
I have been playing around with making updatable views support MERGE, and it looks to be fairly straightforward. I'm intending to support auto-updatable views, WITH CHECK OPTION, and trigger-updatable views, but not views with rules, as I think that would be more trouble than it's worth. Per the SQL standard, if the view isn't auto-updatable, it requires the appropriate INSTEAD OF INSERT/UPDATE/DELETE triggers to perform the merge actions. One limitation with the current patch is that it will only work if the view is either auto-updatable with no INSTEAD OF triggers, or it has a full set of INSTEAD OF triggers for all INSERT/UPDATE/DELETE actions mentioned in the MERGE command. It doesn't support a mix of those 2 cases (i.e., a partial set of INSTEAD OF triggers, such as an INSTEAD OF INSERT trigger only, on an otherwise auto-updatable view). Perhaps it will be possible to overcome that limitation in the future, but I think that it will be hard. In practice though, I think that this shouldn't be very limiting -- I think it's uncommon for people to define INSTEAD OF triggers on auto-updatable views, and if they do, they just need to be sure to provide a full set. Attached is a WIP patch, which I'll add to the next CF. I still need to do more testing, and update the docs, but so far, everything appears to work. Regards, Dean
Attachment
On Thu, 8 Dec 2022 at 10:03, Dean Rasheed <dean.a.rasheed@gmail.com> wrote: > > Attached is a WIP patch, which I'll add to the next CF. I still need > to do more testing, and update the docs, but so far, everything > appears to work. > New patch attached with doc updates and a few other, mostly cosmetic, changes. One notable change is that I realised that the check for rules on the target table needs to be done in the rewriter, rather than the parser, in case expanding a view hierarchy leads to base relations with rules that the parser wouldn't notice. Regards, Dean
Attachment
On Wed, 21 Dec 2022 at 20:04, Dean Rasheed <dean.a.rasheed@gmail.com> wrote: > > New patch attached with doc updates and a few other, mostly cosmetic, changes. > New version fixing a bug in preprocess_targetlist() -- given a simple auto-updatable view that also has INSTEAD OF triggers, subquery pullup of the target may produce PlaceHolderVars in MERGE WHEN clauses, which the former code wasn't expecting to find. Regards, Dean
Attachment
On Fri, 30 Dec 2022 at 12:47, Dean Rasheed <dean.a.rasheed@gmail.com> wrote: > > New version fixing a bug in preprocess_targetlist(). > Rebased version, following 5d29d525ff. Regards, Dean
Attachment
On Sat, 21 Jan 2023 at 11:03, Dean Rasheed <dean.a.rasheed@gmail.com> wrote: > > Rebased version, following 5d29d525ff. > Updated version attached. This needed a little extra tweaking to work following the change to make Vars outer-join aware, so it's worth checking that I understood that properly -- when merging into a trigger-updatable view, the whole-row Var added to the targetlist by the rewriter is nullable by the join added by transform_MERGE_to_join(). The make-Vars-outer-join-aware patch has possibly made this patch's change to preprocess_targetlist() unnecessary, but I left it in just in case, even though I can no longer trigger that failure mode. It feels safer, and more consistent with the code later on in that function. Regards, Dean
Attachment
On Tue, 7 Feb 2023 at 10:03, Dean Rasheed <dean.a.rasheed@gmail.com> wrote: > > Updated version attached. > Rebased version attached. Regards, Dean
Attachment
On Fri, 24 Feb 2023 at 05:43, Dean Rasheed <dean.a.rasheed@gmail.com> wrote: > > Rebased version attached. > Another rebase. Regards, Dean
Attachment
On Mon, 13 Mar 2023 at 13:00, Dean Rasheed <dean.a.rasheed@gmail.com> wrote: > > Another rebase. > And another rebase. Regards, Dean
Attachment
On Sun, 19 Mar 2023 at 09:11, Dean Rasheed <dean.a.rasheed@gmail.com> wrote: > > And another rebase. > Rebased version attached. Aside from a little minor tidying up, I have renamed the new field on the Query to "mergeTargetRelation", which is a little more consistent with the naming of existing fields, and with the "mergeSourceRelation" field that the "WHEN NOT MATCHED BY SOURCE" patch adds. Regards, Dean
Attachment
hi.
Excellent work! regress test passed! The code looks so intuitive!
doc/src/sgml/ref/create_view.sgml.
Do we need to add <<command>MERGE</command> for the following sentence?
If the view or any of its base
relations has an <literal>INSTEAD</literal> rule that causes the
<command>INSERT</command> or <command>UPDATE</command> command
to be rewritten, then
all check options will be ignored in the rewritten query, including any
checks from automatically updatable views defined on top of the relation
with the <literal>INSTEAD</literal> rule.
in src/backend/executor/nodeModifyTable.c line 3800: ExecModifyTable
`
datum = ExecGetJunkAttribute(slot,resultRelInfo->ri_RowIdAttNo,&isNull);
.....
oldtupdata.t_data = DatumGetHeapTupleHeader(datum);
oldtupdata.t_len = HeapTupleHeaderGetDatumLength(oldtupdata.t_data);
`
In ExecGetJunkAttribute(slot,resultRelInfo->ri_RowIdAttNo,&isNull);
does resultRelInfo->ri_RowIdAttNo must be 1 to make sure
DatumGetHeapTupleHeader(datum) works?
(I am not familiar with this part.....)
On Sat, 28 Oct 2023 at 09:35, jian he <jian.universality@gmail.com> wrote: > > hi. > Excellent work! regress test passed! The code looks so intuitive! > Thanks for looking! > doc/src/sgml/ref/create_view.sgml. > Do we need to add <<command>MERGE</command> for the following sentence? > > If the view or any of its base > relations has an <literal>INSTEAD</literal> rule that causes the > <command>INSERT</command> or <command>UPDATE</command> command > to be rewritten, then > all check options will be ignored in the rewritten query, including any > checks from automatically updatable views defined on top of the relation > with the <literal>INSTEAD</literal> rule. > We don't want to include MERGE in that sentence, because MERGE isn't supported on views or tables with rules, but I guess we could add another sentence after that one, to make that clear. > in src/backend/executor/nodeModifyTable.c line 3800: ExecModifyTable > ` > datum = ExecGetJunkAttribute(slot,resultRelInfo->ri_RowIdAttNo,&isNull); > ..... > oldtupdata.t_data = DatumGetHeapTupleHeader(datum); > oldtupdata.t_len = HeapTupleHeaderGetDatumLength(oldtupdata.t_data); > ` > In ExecGetJunkAttribute(slot,resultRelInfo->ri_RowIdAttNo,&isNull); > > does resultRelInfo->ri_RowIdAttNo must be 1 to make sure > DatumGetHeapTupleHeader(datum) works? > (I am not familiar with this part.....) Well, it's not necessarily 1. It's whatever the attribute number of the "wholerow" attribute is, which can vary. "datum" is then set to the value of the "wholerow" attribute, which is the OLD tuple, so using DatumGetHeapTupleHeader() makes sense. This relies on the code in ExecInitModifyTable(), which sets up ri_RowIdAttNo. Regards, Dean
On Sun, 29 Oct 2023 at 17:17, Dean Rasheed <dean.a.rasheed@gmail.com> wrote: > > On Sat, 28 Oct 2023 at 09:35, jian he <jian.universality@gmail.com> wrote: > > > > Do we need to add <<command>MERGE</command> for the following sentence? > > > We don't want to include MERGE in that sentence, because MERGE isn't > supported on views or tables with rules, but I guess we could add > another sentence after that one, to make that clear. > Here's an updated patch doing that, plus another couple of minor updates to that page. I also noticed that the code to detect rules ought to ignore disabled rules, so I've updated it to do so, and added a new regression test to cover that case. Arguably that's a pre-existing bug, so the fix could be extracted and applied separately, but I'm not sure that it's worth the effort. Regards, Dean
Attachment
On Mon, 30 Oct 2023 at 15:04, Dean Rasheed <dean.a.rasheed@gmail.com> wrote: > > On Sun, 29 Oct 2023 at 17:17, Dean Rasheed <dean.a.rasheed@gmail.com> wrote: > > > > On Sat, 28 Oct 2023 at 09:35, jian he <jian.universality@gmail.com> wrote: > > > > > > Do we need to add <<command>MERGE</command> for the following sentence? > > > > > We don't want to include MERGE in that sentence, because MERGE isn't > > supported on views or tables with rules, but I guess we could add > > another sentence after that one, to make that clear. > > > > Here's an updated patch doing that, plus another couple of minor > updates to that page. > > I also noticed that the code to detect rules ought to ignore disabled > rules, so I've updated it to do so, and added a new regression test to > cover that case. > > Arguably that's a pre-existing bug, so the fix could be extracted and > applied separately, but I'm not sure that it's worth the effort. CFBot shows that the patch does not apply anymore as in [1]: === Applying patches on top of PostgreSQL commit ID 55627ba2d334ce98e1f5916354c46472d414bda6 === === applying patch ./support-merge-into-view-v10.patch .... patching file src/backend/executor/nodeModifyTable.c ... Hunk #7 FAILED at 2914. ... 1 out of 15 hunks FAILED -- saving rejects to file src/backend/executor/nodeModifyTable.c.rej Please post an updated version for the same. [1] - http://cfbot.cputube.org/patch_46_4076.log Regards, Vignesh
On Fri, 26 Jan 2024 at 15:02, vignesh C <vignesh21@gmail.com> wrote: > > CFBot shows that the patch does not apply anymore as in [1]: > Rebased version attached. Regards, Dean
Attachment
Thanks for working on this. The patch looks well finished. I didn't
try to run it, though. I gave it a read and found nothing to complain
about, just these two pretty minor comments:
On 2024-Jan-26, Dean Rasheed wrote:
> diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
> new file mode 100644
> index 13a9b7d..5a99e4a
> --- a/src/backend/executor/execMain.c
> +++ b/src/backend/executor/execMain.c
> @@ -1021,11 +1021,13 @@ InitPlan(QueryDesc *queryDesc, int eflag
> * CheckValidRowMarkRel.
> */
> void
> -CheckValidResultRel(ResultRelInfo *resultRelInfo, CmdType operation)
> +CheckValidResultRel(ResultRelInfo *resultRelInfo, CmdType operation,
> + List *mergeActions)
> {
I suggest to add commentary explaining the new argument of this function.
> @@ -1080,6 +1083,51 @@ CheckValidResultRel(ResultRelInfo *resul
> RelationGetRelationName(resultRel)),
> errhint("To enable deleting from the view, provide an INSTEAD OF DELETE trigger or
anunconditional ON DELETE DO INSTEAD rule.")));
> break;
> + case CMD_MERGE:
> +
> + /*
> + * Must have a suitable INSTEAD OF trigger for each MERGE
> + * action. Note that the error hints here differ from
> + * above, since MERGE doesn't support rules.
> + */
> + foreach(lc, mergeActions)
> + {
> + MergeAction *action = (MergeAction *) lfirst(lc);
> +
> + switch (action->commandType)
> + {
> + case CMD_INSERT:
> + if (!trigDesc || !trigDesc->trig_insert_instead_row)
> + ereport(ERROR,
> + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("cannot insert into view \"%s\"",
> + RelationGetRelationName(resultRel)),
> + errhint("To enable inserting into the view using MERGE, provide an
INSTEADOF INSERT trigger.")));
Looking at the comments in ereport_view_not_updatable and the code
coverate reports, it appears that these checks here are dead code? If
so, maybe it would be better to turn them into elog() calls? I don't
mean to touch the existing code, just that I don't see that it makes
sense to introduce the new ones as ereport().
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"La persona que no quería pecar / estaba obligada a sentarse
en duras y empinadas sillas / desprovistas, por cierto
de blandos atenuantes" (Patricio Vogel)
On Fri, 26 Jan 2024 at 16:48, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> Thanks for working on this. The patch looks well finished. I didn't
> try to run it, though. I gave it a read and found nothing to complain
> about, just these two pretty minor comments:
>
Thanks for reviewing.
> > -CheckValidResultRel(ResultRelInfo *resultRelInfo, CmdType operation)
> > +CheckValidResultRel(ResultRelInfo *resultRelInfo, CmdType operation,
> > + List *mergeActions)
> > {
>
> I suggest to add commentary explaining the new argument of this function.
>
OK.
> > @@ -1080,6 +1083,51 @@ CheckValidResultRel(ResultRelInfo *resul
> > RelationGetRelationName(resultRel)),
> > errhint("To enable deleting from the view, provide
anINSTEAD OF DELETE trigger or an unconditional ON DELETE DO INSTEAD rule.")));
> > break;
> > + case CMD_MERGE:
> > +
> > + /*
> > + * Must have a suitable INSTEAD OF trigger for each MERGE
> > + * action. Note that the error hints here differ from
> > + * above, since MERGE doesn't support rules.
> > + */
> > + foreach(lc, mergeActions)
> > + {
> > + MergeAction *action = (MergeAction *) lfirst(lc);
> > +
> > + switch (action->commandType)
> > + {
> > + case CMD_INSERT:
> > + if (!trigDesc || !trigDesc->trig_insert_instead_row)
> > + ereport(ERROR,
> > +
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> > + errmsg("cannot insert into
view\"%s\"",
> > +
RelationGetRelationName(resultRel)),
> > + errhint("To enable inserting
intothe view using MERGE, provide an INSTEAD OF INSERT trigger.")));
>
> Looking at the comments in ereport_view_not_updatable and the code
> coverate reports, it appears that these checks here are dead code? If
> so, maybe it would be better to turn them into elog() calls? I don't
> mean to touch the existing code, just that I don't see that it makes
> sense to introduce the new ones as ereport().
>
Yeah, for all practical purposes, that check in CheckValidResultRel()
has been dead code since d751ba5235, but I think it's still worth
doing, and if we're going to do it, we should do it properly. I don't
like using elog() in some cases and ereport() in others, but I also
don't like having that much duplicated code between this and the
rewriter (and this patch doubles the size of that block).
A neater solution is to export the rewriter functions and use them in
CheckValidResultRel(). All these checks can then be reduced to
if (!view_has_instead_trigger(...))
error_view_not_updatable(...)
which eliminates a lot of duplicated code and means that we now have
just one place that throws these errors.
Regards,
Dean
Attachment
On 2024-Jan-29, Dean Rasheed wrote: > Yeah, for all practical purposes, that check in CheckValidResultRel() > has been dead code since d751ba5235, but I think it's still worth > doing, and if we're going to do it, we should do it properly. I don't > like using elog() in some cases and ereport() in others, but I also > don't like having that much duplicated code between this and the > rewriter (and this patch doubles the size of that block). > > A neater solution is to export the rewriter functions and use them in > CheckValidResultRel(). All these checks can then be reduced to > > if (!view_has_instead_trigger(...)) > error_view_not_updatable(...) > > which eliminates a lot of duplicated code and means that we now have > just one place that throws these errors. This looks quite nice, thanks. LGTM. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "No me acuerdo, pero no es cierto. No es cierto, y si fuera cierto, no me acuerdo." (Augusto Pinochet a una corte de justicia)
On Tue, 30 Jan 2024 at 11:58, Alvaro Herrera <alvherre@alvh.no-ip.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
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)
On Thu, 29 Feb 2024 at 09:50, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > By all means let's get the feature out there. It's not a frequently > requested thing but it does seem to come up. > Pushed. Thanks for reviewing. Regards, Dean