On Fri, Apr 30, 2021 at 10:49 AM Bharath Rupireddy <bharath.rupireddyforpostg...@gmail.com> wrote: > > On Fri, Apr 30, 2021 at 7:07 AM Justin Pryzby <pry...@telsasoft.com> wrote: > > > > On Thu, Apr 29, 2021 at 02:39:42PM -0400, Alvaro Herrera wrote: > > > On 2021-Apr-29, Tom Lane wrote: > > > > Alvaro Herrera <alvhe...@alvh.no-ip.org> writes: > > > > > I'd do it like this. Note I removed an if/else block in addition to > > > > > your changes. > > > > > > > > > I couldn't convince myself that this is worth pushing though; either > > > > > we > > > > > push it to all branches (which seems unwarranted) or we create > > > > > back-patching hazards. > > > > > > > > Yeah ... an advantage of the if/else coding is that it'd likely be > > > > simple to extend to cover additional statement types, should we ever > > > > wish to do that. The rendering you have here is nice and compact, > > > > but it would not scale up well. > > > > > > That makes sense. But that part is not in Amul's patch -- he was only > > > on about removing the is_foreign_table Boolean. If I remove the if/else > > > block change, does the rest of the patch looks something we'd want to > > > have? I kinda agree that the redundant variable is "ugly". Is it worth > > > removing? My hunch is no. > > > > Getting rid of a redundant, boolean variable is good not because it's more > > efficient but because it's one fewer LOC to read and maintain (and an > > opportunity for inconsistency, I suppose). > > Yes. > > > Also, this is a roundabout and too-verbose way to invert a boolean: > > | transformCheckConstraints(&cxt, !is_foreign_table ? true : false); > > I agree to remove only the redundant variable, is_foreign_table but > not the if else block as Tom said: it's not scalable.
+1. Regards, Amul