Обсуждение: Re: [HACKERS] pgsql: Avoid coercing a whole-row variable that is already coerced
Re: [HACKERS] pgsql: Avoid coercing a whole-row variable that is already coerced
От
Amit Khandekar
Дата:
Bringing here the mail thread from pgsql-committers regarding this commit : commit 1c497fa72df7593d8976653538da3d0ab033207f Author: Robert Haas <rhaas@postgresql.org> Date: Thu Oct 12 17:10:48 2017 -0400 Avoid coercing a whole-row variable that is already coerced. Marginal efficiency and beautification hack. I'm not sure whether this case ever arises currently, but the pending patch for update tuple routing will cause it to arise. Amit Khandekar Discussion: http://postgr.es/m/CAJ3gD9cazfppe7-wwUbabPcQ4_0SubkiPFD1+0r5_DkVNWo5yg@mail.gmail.com Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote: > Robert Haas <rhaas(at)postgresql(dot)org> wrote: > > Avoid coercing a whole-row variable that is already coerced. > > This logic seems very strange and more than likely buggy: the > added coerced_var flag feels like the sort of action at a distance > that is likely to have unforeseen side effects. > > I'm not entirely sure what the issue is here, but if you're concerned > about not applying two ConvertRowtypeExprs in a row, why not have the > upper one just strip off the lower one? We handle, eg, nested > RelabelTypes that way. We kind of do a similar thing. When a ConvertRowtypeExpr node is encountered, we create a new ConvertRowtypeExpr that points to a new var, and return this new ConvertRowtypeExpr instead of the existing one. So we actually replace the old with a new one. But additionally, we also want to change the vartype of the new var to context->to_rowtype. I think you are worried specifically about coerced_var causing unexpected regression in existing scenarios, such as : context->coerced_var getting set and prematurely unset in recursive scenarios. But note that, when we call map_variable_attnos_mutator() just after setting context->coerced_var = true, map_variable_attnos_mutator() won't recurse further, because it is always called with a Var, which does not have any further arguments to process. So coerced_var won't be again changed until we return from map_variable_attnos_mutator(). The only reason why we chose to call map_variable_attnos_mutator() with a Var is so that we can re-use the code that converts the whole row var. One thing we can do is : instead of calling map_variable_attnos_mutator(), convert the var inside the if block for "if (IsA(node, ConvertRowtypeExpr))". Please check the attached patch. There, I have avoided coerced_var context variable. -- Thanks, -Amit Khandekar EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Вложения
Re: [HACKERS] pgsql: Avoid coercing a whole-row variable that is already coerced
От
Robert Haas
Дата:
On Fri, Oct 13, 2017 at 5:57 AM, Amit Khandekar <amitdkhan.pg@gmail.com> wrote: > One thing we can do is : instead of calling > map_variable_attnos_mutator(), convert the var inside the if block for > "if (IsA(node, ConvertRowtypeExpr))". Please check the attached patch. > There, I have avoided coerced_var context variable. Tom, is this more like what you have in mind? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Robert Haas <robertmhaas@gmail.com> writes: > On Fri, Oct 13, 2017 at 5:57 AM, Amit Khandekar <amitdkhan.pg@gmail.com> wrote: >> One thing we can do is : instead of calling >> map_variable_attnos_mutator(), convert the var inside the if block for >> "if (IsA(node, ConvertRowtypeExpr))". Please check the attached patch. >> There, I have avoided coerced_var context variable. > Tom, is this more like what you have in mind? It's better ... but after reading the patched code, a lot of my remaining beef is with the lack of clarity of the comments. You need ESP to understand what the function is trying to accomplish and what the constraints are. I'll take a whack at improving that and push. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgsql: Avoid coercing a whole-row variable that isalready coerced
От
Robert Haas
Дата:
On Fri, Oct 13, 2017 at 12:56 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > It's better ... but after reading the patched code, a lot of my remaining > beef is with the lack of clarity of the comments. You need ESP to > understand what the function is trying to accomplish and what the > constraints are. I'll take a whack at improving that and push. OK, thanks. The good thing is, now that we know you have ESP, you can use it with the Ouija board Andres used to decide whether to apply sizeof to the variable or the type. That's got to be a win. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers