On Sun, Jun 11, 2017 at 6:25 PM, Noah Misch <noah@leadboat.com> wrote:
> While completing my annual src/backend/nodes/*funcs.c audit, I noticed defects
> in commit 18ce3a4 changes to RangeTblEntry:
>
> 1. Field relid is under a comment saying it is valid for RTE_RELATION only.
The comment is out of date. Here's a fix for that.
> Fields coltypes, coltypmods and colcollations are under a comment saying
> they are valid for RTE_VALUES and RTE_CTE only. But _outRangeTblEntry()
> treats all of the above as valid for RTE_NAMEDTUPLESTORE. Which is right?
The comment is wrong. In passing I also noticed that RTE_TABLEFUNC
also uses coltypes et al and is not mentioned in that comment. Here's
a fix for both omissions.
> 2. New fields enrname and enrtuples are set only for RTE_NAMEDTUPLESTORE, yet
> they're under the comment for RTE_VALUES and RTE_CTE. This pair needs its
> own comment.
Right. The attached patch fixes that too.
> 3. Each of _{copy,equal,out,read}RangeTblEntry() silently ignores enrtuples.
> _equalRangeTblEntry() ignores enrname, too. In each case, the function
> should either use the field or have a comment to note that skipping the
> field is intentional. Which should it be?
Ignoring enrname in _equalRangeTblEntry is certainly a bug, and the
attached adds it. I also noticed that _copyRangeTleEntry had enrname
but not in the same order as the struct's members, which seems to be
an accidental deviation from the convention, so I moved it in the
attached.
Ignoring enrtuples is probably also wrong, but ...
> This fourth point is not necessarily a defect: I wonder if RangeTblEntry is
> the right place for enrtuples. It's a concept regularly seen in planner data
> structures but not otherwise seen at parse tree level.
I agree that this is strange. Perhaps
set_namedtuplestore_size_estimates should instead look up the
EphemeralNamedRelation by rte->enrname to find its way to
enr->md.enrtuples, but I'm not sure off the top of my head how it
should get its hands on the QueryEnvironment required to do that. I
will look into this on Monday, but other ideas/clues welcome...
--
Thomas Munro
http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers