On Sun, Jun 11, 2017 at 6:25 PM, Noah Misch <n...@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

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

Attachment: fixes-for-enr-rte-review-v1.patch
Description: Binary data

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to