Re: Adding OLD/NEW support to RETURNING
От | Dean Rasheed |
---|---|
Тема | Re: Adding OLD/NEW support to RETURNING |
Дата | |
Msg-id | CAEZATCVF-nZQBZ4GOHSgn4ccjbymA37uMVVHJhgP_57ZGJyQXg@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: Adding OLD/NEW support to RETURNING (jian he <jian.universality@gmail.com>) |
Ответы |
Re: Adding OLD/NEW support to RETURNING
(Dean Rasheed <dean.a.rasheed@gmail.com>)
|
Список | pgsql-hackers |
On Mon, 25 Mar 2024 at 00:00, jian he <jian.universality@gmail.com> wrote: > > hi, some minor issues I found out. > > the ReplaceReturningVarsFromTargetList related comment > should be placed right above the function ReplaceReturningVarsFromTargetList, > not above ReplaceReturningVarsFromTargetList_context? Hmm, well there are a mix of possible styles for this kind of function. Sometimes the outer function comes first, immediately after the function comment, and then the callback function comes after that. That has the advantage that all documentation comments related to the top-level input arguments are next to the function that takes them. Also, this ordering means that you naturally read it in the order in which it is initially executed. The other style, putting the callback function first has the advantage that you can more immediately see what the function does, since it's usually the callback that contains the interesting logic. rewriteManip.c has examples of both styles, but in this case, since ReplaceReturningVarsFromTargetList() is similar to ReplaceVarsFromTargetList(), I opted to copy its style. > struct ReplaceReturningVarsFromTargetList_context adds some comments > about new_result_relation would be great. I substantially rewrote that function in the v6 patch. As part of that, I renamed "new_result_relation" to "new_target_varno", which more closely matches the existing "target_varno" argument, and I added comments about what it's for to the top-level function comment block. > /* INDEX_VAR is handled by default case */ > this comment appears in execExpr.c and execExprInterp.c. > need to move to default case's switch default case? No, I think it's fine as it is. Its current placement is where you might otherwise expect to find a "case INDEX_VAR:" block of code, and it's explaining why there isn't one there, and where to look instead. Moving it into the switch default case would lose that effect, and I think it would reduce the code's readability. > /* > * set_deparse_context_plan - Specify Plan node containing expression > * > * When deparsing an expression in a Plan tree, we might have to resolve > * OUTER_VAR, INNER_VAR, or INDEX_VAR references. To do this, the caller must > * provide the parent Plan node. > ... > */ > does the comment in set_deparse_context_plan need to be updated? In the v6 patch, I moved the code change from set_deparse_context_plan() down into set_deparse_plan(), because I thought that would catch more cases, but thinking about it some more, that wasn't necessary, since it won't change when moving up and down the ancestor tree. So in v7, I've moved it back and updated the comment. > + * buildNSItemForReturning - > + * add a ParseNamespaceItem for the OLD or NEW alias in RETURNING. > + */ > +static void > +addNSItemForReturning(ParseState *pstate, const char *aliasname, > + VarReturningType returning_type) > comment "buildNSItemForReturning" should be "addNSItemForReturning"? Yes, well spotted. Fixed in v7. > [in expandRTE()] > > - * rtindex, sublevels_up, and location are the varno, varlevelsup, and location > - * values to use in the created Vars. Ordinarily rtindex should match the > - * actual position of the RTE in its rangetable. > + * rtindex, sublevels_up, returning_type, and location are the varno, > + * varlevelsup, varreturningtype, and location values to use in the created > + * Vars. Ordinarily rtindex should match the actual position of the RTE in > + * its rangetable. > we already updated the comment in expandRTE. > but it seems we only do RTE_RELATION, some part of RTE_FUNCTION. > do we need > ` > varnode->varreturningtype = returning_type; > ` > for other `rte->rtekind` when there is a makeVar? > > (I don't understand this part, in the case where rte->rtekind is > RTE_SUBQUERY, if I add `varnode->varreturningtype = returning_type;` > the tests still pass. In the v6 patch, I already added code to ensure that it's set in all cases, though I don't think it's strictly necessary. returning_type can only have a non-default value for the target RTE, which can't currently be any of those other RTE kinds, but nonetheless it seemed better from a consistency point-of-view, and to make it more future-proof. v7 patch attached, with those updates. Regards, Dean
Вложения
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Amit KapilaДата:
Сообщение: Re: pgsql: Track last_inactive_time in pg_replication_slots.