From: Tom Lane <tgl@sss.pgh.pa.us>
To: Tomas Vondra <tomas.vondra@enterprisedb.com>
cc: Noah Misch <noah@leadboat.com>, Dean Rasheed <dean.a.rasheed@gmail.com>,
        Justin Pryzby <pryzby@telsasoft.com>,
        PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: PoC/WIP: Extended statistics on expressions
In-reply-to: <2392f864-7ec1-38bb-1e78-71a9b2d78a95@enterprisedb.com>
References: 
 <CAEZATCWYCY=2yEyxa6BwimCPQ_RZHy5GOb6U6i6dGHGQdjQESQ@mail.gmail.com>
 <e4556e47-940d-58a6-73fd-16c5815dd2cd@enterprisedb.com>
 <CAEZATCW2WHfsrbDLzwMvD+kOsBemLhvKwoHGhzR7STtac74XTg@mail.gmail.com>
 <a7d17aec-234d-c7f2-bfdc-99c87767a507@enterprisedb.com>
 <1ed534fb-b42b-f008-35ee-4eb930b0088f@enterprisedb.com>
 <CAEZATCV-JgFzb6aCJxApXg302u4vw+ajj702RHaBVkS_4JPVRg@mail.gmail.com>
 <2cb8a7cd-664f-8681-6ce9-790a50e3c78f@enterprisedb.com>
 <CAEZATCXsfFcXW_A1XCd5QK6zT2M_V+P3aPSktjibHHPzoV+Y-A@mail.gmail.com>
 <096d26e1-f053-a039-047d-a050dbb80ac6@enterprisedb.com>
 <078de413-8e4e-343e-fac1-eb61fd389925@enterprisedb.com>
 <20210606053740.GB297923@rfd.leadboat.com>
 <4c98203c-8364-bb76-b502-d7b323ba55ba@enterprisedb.com>
 <660936.1623007056@sss.pgh.pa.us>
 <2392f864-7ec1-38bb-1e78-71a9b2d78a95@enterprisedb.com>
Comments: In-reply-to Tomas Vondra <tomas.vondra@enterprisedb.com>
	message dated "Sun, 06 Jun 2021 22:01:23 +0200"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <703785.1623010132.1@sss.pgh.pa.us>
Date: Sun, 06 Jun 2021 16:08:52 -0400
Message-ID: <703786.1623010132@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/703786.1623010132%40sss.pgh.pa.us>
Precedence: bulk

Tomas Vondra <tomas.vondra@enterprisedb.com> writes:
> On 6/6/21 9:17 PM, Tom Lane wrote:
>> I'm curious about how come the buildfarm didn't notice this.  The
>> animals using COPY_PARSE_PLAN_TREES should have failed.  The fact
>> that they didn't implies that there's no test case that makes use
>> of a nonzero value for this field, which seems like a testing gap.

> AFAICS the reason is pretty simple - the COPY_PARSE_PLAN_TREES checks
> look like this:
> ...
> But if the field is missing from all the functions, equal() can't detect
> that copyObject() did not actually copy it.

Right, that code would only detect a missing copyfuncs.c line if
equalfuncs.c did have the line, which isn't all that likely.  However,
we then pass the copied node on to further processing, which in principle
should result in visible failures when copyfuncs.c is missing a line.

I think the reason it didn't is that the transformed field would always
be zero (false) in grammar output.  We could only detect a problem if
we copied already-transformed nodes and then used them further.  Even
then it *might* not fail, because the consequence would likely be an
extra round of parse analysis on the expressions, which is likely to
be a no-op.

Not sure if there's a good way to improve that.  I hope sometime soon
we'll be able to auto-generate these functions, and then the risk of
this sort of mistake will go away (he says optimistically).

			regards, tom lane