Обсуждение: Union-ifying RangeTblEntry

Hi all

I'm about to have to add _another_ flag to RangeTblEntry, to track
row-security expansion.

In the process I noticed the comment:
   /*    * XXX the fields applicable to only some rte kinds should be    * merged into a union.  I didn't do this yet
becausethe diffs    * would impact a lot of code that is being actively worked on.    * FIXME someday.    */
 

and it struck me that the end of the 9.4 commitfest might be a
reasonable time to do this now that PstgreSQL is subject to "pulsed"
development with commitfests.

As part of that, a number of the flag fields on RangeTblEntry into a
bitfield.

Comments?

-- Craig Ringer                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Jan 28, 2014 at 1:18 AM, Craig Ringer <craig@2ndquadrant.com> wrote:
> I'm about to have to add _another_ flag to RangeTblEntry, to track
> row-security expansion.
>
> In the process I noticed the comment:
>
>     /*
>      * XXX the fields applicable to only some rte kinds should be
>      * merged into a union.  I didn't do this yet because the diffs
>      * would impact a lot of code that is being actively worked on.
>      * FIXME someday.
>      */
>
> and it struck me that the end of the 9.4 commitfest might be a
> reasonable time to do this now that PstgreSQL is subject to "pulsed"
> development with commitfests.
>
> As part of that, a number of the flag fields on RangeTblEntry into a
> bitfield.
>
> Comments?

I'd be more inclined to just remove the comment.  Does a RangeTblEntry
really use enough memory that we need to conserve bytes there?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Jan 28, 2014 at 1:18 AM, Craig Ringer <craig@2ndquadrant.com> wrote:
>> In the process I noticed the comment:
>> 
>> /*
>> * XXX the fields applicable to only some rte kinds should be
>> * merged into a union.  I didn't do this yet because the diffs
>> * would impact a lot of code that is being actively worked on.
>> * FIXME someday.
>> */

> I'd be more inclined to just remove the comment.  Does a RangeTblEntry
> really use enough memory that we need to conserve bytes there?

It doesn't; RTEs aren't that big, and more to the point, there aren't that
many of them per query.  I think I wrote that comment, many years ago,
when memory was more precious ... but even then it was more out of
neatnik-ism than any real need to conserve space.

A bigger consideration is the risk of introducing bugs: how certain are
we that no code ever touches fields that are not supposed to be used for
the current rtekind?  I'm sure not.  Right now, it's safe to assume that
e.g. subquery is NULL if it's not an RTE_SUBQUERY, and I think there
are probably places that depend on that.

And lastly, we'd be touching enough code to make such a change a serious
PITA for back-patching, and for third-party modules.  In return for not
much.

So yeah, let's just drop the comment in favor of a note that irrelevant
fields should be zero/null.
        regards, tom lane