Message-ID: <9818bd29-3eff-40c5-ad6a-8d89ba8c7acc@illuminatedcomputing.com>
Date: Fri, 16 Feb 2024 12:36:24 -0800
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: automating RangeTblEntry node support
Content-Language: en-US
To: pgsql-hackers@lists.postgresql.org
References: <4b27fc50-8cd6-46f5-ab20-88dbaadca645@eisentraut.org>
 <859d6155-e361-4a05-8db3-4aa1f007ff28@eisentraut.org>
From: Paul Jungwirth <pj@illuminatedcomputing.com>
In-Reply-To: <859d6155-e361-4a05-8db3-4aa1f007ff28@eisentraut.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: <https://www.postgresql.org/message-id/9818bd29-3eff-40c5-ad6a-8d89ba8c7acc%40illuminatedcomputing.com>
Precedence: bulk

On 1/15/24 02:37, Peter Eisentraut wrote:
> In this updated patch set, I have also added the treatment of the Constraint type.  (I also noted 
> that the manual read/write functions for the Constraint type are out-of-sync again, so simplifying 
> this would be really helpful.)  I have also added commit messages to each patch.
> 
> The way I have re-ordered the patch series now, I think patches 0001 through 0003 are candidates for 
> inclusion after review, patch 0004 still needs a bit more analysis and testing, as described therein.

I had to apply the first patch by hand (on 9f13376396), so this looks due for a rebase. Patches 2-4 
applied fine.

Compiles & passes tests after each patch.

The overall idea seems like a good improvement to me.

A few remarks about cleaning up the RangeTblEntry comments:

After the fourth patch we have a "Fields valid in all RTEs" comment twice in the struct, once at the 
top and once at the bottom. It's fine IMO but maybe the second could be "More fields valid in all RTEs"?

The new order of fields in RangleTblEntry matches the intro comment, which seems like another small 
benefit.

It seems like we are moving away from ever putting RTEKind-specific fields into a union as suggested 
by the FIXME comment here. It was written in 2002. Is it time to remove it?

This now needs to say "above" not "below":

     /*
      * join_using_alias is an alias clause attached directly to JOIN/USING. It
      * is different from the alias field (below) in that it does not hide the
      * range variables of the tables being joined.
      */
     Alias      *join_using_alias pg_node_attr(query_jumble_ignore);

Re bloating the serialization output, we could leave this last patch until after the work on that 
other thread is done to skip default-valued items.

Yours,

-- 
Paul              ~{:-)
pj@illuminatedcomputing.com