Re: Adding OLD/NEW support to RETURNING - Mailing list pgsql-hackers
From | Dean Rasheed |
---|---|
Subject | Re: Adding OLD/NEW support to RETURNING |
Date | |
Msg-id | CAEZATCWeME=8p7-nChF5TQXxJ5At4mZ=n2yz+kNPk14VPAUV-A@mail.gmail.com Whole thread Raw |
In response to | Re: Adding OLD/NEW support to RETURNING (jian he <jian.universality@gmail.com>) |
Responses |
Re: Adding OLD/NEW support to RETURNING
|
List | pgsql-hackers |
On Mon, 5 Aug 2024 at 12:46, jian he <jian.universality@gmail.com> wrote: > > took me a while to understand the changes in rewriteHandler.c, rewriteManip.c > rule over updateable view still works, but I didn't check closely with > rewriteRuleAction. > i think I understand rewriteTargetView and subroutines. > > * In addition, the caller must provide result_relation, the index of the > * target relation for an INSERT/UPDATE/DELETE/MERGE. This is needed to > * handle any OLD/NEW RETURNING list Vars referencing target_varno. When such > * Vars are expanded, varreturningtype is copied onto any replacement Vars > * that reference result_relation. In addition, if the replacement expression > * from the targetlist is not simply a Var referencing result_relation, we > * wrap it in a ReturningExpr node, to force it to be NULL if the OLD/NEW row > * doesn't exist. > > "the index of the target relation for an INSERT/UPDATE/DELETE/MERGE", > here, "target relation" I think people may be confused whether it > refers to view relation or the base relation. > I think here the target relation is the base relation (rtekind == RTE_RELATION) Yes, it's the result relation in the rewritten query. I've updated that comment to try to make that clearer. Basically, if a replacement Var refers to the new result relation in the rewritten query, then its varreturningtype needs to be set correctly. Otherwise, if it refers to some other relation, its varreturningtype shouldn't be changed, but it does need to be wrapped in a ReturningExpr node, if the original Var had a non-default varreturningtype, so that it evaluates as NULL if the old/new row doesn't exist. > " to force it to be NULL if the OLD/NEW row doesn't exist." > i think this happen in execExpr.c? > maybe > " to force it to be NULL if the OLD/NEW row doesn't exist, see execExpr.c" OK, I've updated it to just say that this causes the executor to return NULL if the old/new row doesn't exist. There are multiple places in the executor that actually make that happen, so it doesn't make sense to just refer to one place. > overall, looks good to me. Thanks for reviewing. I'm pretty happy with the patch now, but I was just thinking about the wholerow case a little more, and I think it's worth changing the way that's handled. Previously, if you wrote something like "RETURNING old", and the old row didn't exist, it would return an all-NULL record (displayed as something like '(,,,,)'), but I don't think that's really right. I think it should actually return NULL. I think that's more consistent with the way "non-existent" is generally handled, for example in a query like "SELECT t1, t2 FROM t1 OUTER JOIN t2 ON ...". It's pretty trivial, but it does involve changing code in 2 places (the first for regular tables, and the second for updatable views): 1. ExecEvalWholeRowVar() now checks EEO_FLAG_OLD_IS_NULL and EEO_FLAG_NEW_IS_NULL. This makes it more consistent with ExecEvalSysVar(), so I added the same Asserts. 2. ReplaceVarsFromTargetList() now wraps the RowExpr node created in the wholerow case in a ReturningExpr. That's consistent with the function's comment: "if the replacement expression from the targetlist is not simply a Var referencing result_relation, it is wrapped in a ReturningExpr node". Both those changes seem quite natural and consistent, and I think the resulting test output looks much nicer. Regards, Dean
Attachment
pgsql-hackers by date: