Feedback-ID: ie0a040ee:Fastmail
Message-ID: <ad822409-c1c9-400a-b214-9d7e15b4a426@eisentraut.org>
Date: Fri, 26 Jan 2024 14:42:17 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: tablecmds.c/MergeAttributes() cleanup
Content-Language: en-US
To: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>,
 Alvaro Herrera <alvherre@alvh.no-ip.org>
References: <20230829112028.qhzse7chs3swqwqh@alvherre.pgsql>
 <60beec03-f14c-ea46-8e5f-f68773ce83f9@eisentraut.org>
 <a7bee080-ca35-f49e-1ef1-41484f529c37@eisentraut.org>
 <dd2a1ed7-7a8b-435c-a11d-99275eca18dc@eisentraut.org>
 <12e27bdb-1a3d-450f-923b-c36feea24f01@eisentraut.org>
 <CAExHW5vz7A-skzt05=4frFx9-VPjfjK4jKQZT7ufRNh4J7=xmQ@mail.gmail.com>
From: Peter Eisentraut <peter@eisentraut.org>
In-Reply-To: 
 <CAExHW5vz7A-skzt05=4frFx9-VPjfjK4jKQZT7ufRNh4J7=xmQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/ad822409-c1c9-400a-b214-9d7e15b4a426%40eisentraut.org>
Precedence: bulk

On 24.01.24 07:27, Ashutosh Bapat wrote:
> While working on identity support and now while looking at the
> compression problem you referred to, I found MergeAttribute() to be
> hard to read. It's hard to follow high level logic in that function
> since the function is not modular. I took a stab at modularising a
> part of it. Attached is the resulting patch series.
> 
> 0001 is your patch as is
> 0002 is pgindent fix and also fixing what I think is a typo/thinko
> from 0001. If you are fine with the changes, 0002 should be merged
> into 0003.
> 0003 separates the part of code merging a child attribute to the
> corresponding inherited attribute into its own function.
> 0004 does the same for code merging inherited attributes incrementally.
> 
> I have kept 0003 and 0004 separate in case we pick one and not the
> other. But they can be committed as a single commit.

I have committed all this.  These are great improvements.

(One little change I made to your 0003 and 0004 patches is that I kept 
the check whether the new column matches an existing one by name in 
MergeAttributes().  I found that pushing that down made the logic in 
MergeAttributes() too hard to follow.  But it's pretty much the same.)