Re: minor fix for acquire_inherited_sample_rows - Mailing list pgsql-hackers

Hi.

On 2018/04/24 0:16, Alvaro Herrera wrote:
> Hello Amit
> 
> Amit Langote wrote:
> 
>> acquire_inherited_sample_rows() currently uses equalTupleDescs() being
>> false as the condition for going to tupconv.c to determine whether tuple
>> conversion is needed.  But equalTupleDescs() will always return false if
>> it's passed TupleDesc's of two different tables, which is the most common
>> case here.  So I first thought we should just unconditionally go to
>> tupconv.c, but there is still one case where we don't need to, which is
>> the case where the child table is same as the parent table.  However, it
>> would be much cheaper to just check if the relation OIDs are different
>> instead of calling equalTupleDescs, which the attached patch teaches it to do.
> 
> When (not if) we get around to updating equalTupleDescs to cope, we will
> need this call right where it is (and we'd have a hard time finding this
> potential callsite later, I think).

Given what equalTupleDescs was invented for (commit a152ebeec), reducing
it down to what can be sensibly used for checking whether tuple conversion
is needed between a parent and child will, I'm afraid, make it useless for
its original purpose.  Just looking at a few checks that are now in it,
for example:

    for (i = 0; i < tupdesc1->natts; i++)
    {
        Form_pg_attribute attr1 = TupleDescAttr(tupdesc1, i);
        Form_pg_attribute attr2 = TupleDescAttr(tupdesc2, i);

        <...snip...>

        if (attr1->attislocal != attr2->attislocal)
            return false;
        if (attr1->attinhcount != attr2->attinhcount)
            return false;
        <...snip...>
        /* attacl, attoptions and attfdwoptions are not even present... */
    }

attislocal and attinhcount obviously can't be same for parent and a child.
 Also, the comment above seems to assume that this function is being
called from the only place it was designed for (RelationClearRelation).

Further down:

    if (tupdesc1->constr != NULL)
    {
        TupleConstr *constr1 = tupdesc1->constr;
        TupleConstr *constr2 = tupdesc2->constr;

        <...snip...>
        n = constr1->num_defval;
        if (n != (int) constr2->num_defval)
            return false;
        for (i = 0; i < n; i++)
        {
            AttrDefault *defval1 = constr1->defval + i;
            AttrDefault *defval2 = constr2->defval;

            <...snip...>
            if (strcmp(defval1->adbin, defval2->adbin) != 0)
                return false;

Now this last one will always return false, because the location *appears*
to be different in this usage.  We'll have to believe that this test
returns true in at least the cases for which equalTupleDescs() was
designed for.

Then there is code further down that checks other details being equal like
constr->matching, constr->check, etc.  For CHECK constraints, we again
check ccbin, which might have different location values, their inheritance
status, etc.


To summarize, I think equalTupleDescs()'s implementation relies on lots of
details to match between passed-in descriptors (down to basically
everything) for them to be considered "logically" equal.  Not to mention
that "logically" here is for relcache.c's or RelationClearRelation()'s
purposes.

We're considering using this to determine if we need to convert tuples
between parent and child/partition and for that we only need to check if
attributes of the same name appear in the same physical position in both
parent and child.  Rest of the details are either guaranteed same
(individual attributes's types, typemod, typcollate, etc.) or irrelevant
to comparison (their atthasdef, default expressions, check constraints,
etc.).  I proposed to implement such a logic in tupconv.c itself [1],
which I'll re-propose for PG 12.

> If this were a hot spot I would be
> happy to change it, but it's not so I'd rather leave it alone.

Sure, but if we adopt some solution to optimize the check for whether we
need to convert tuples for a hot spot, say, ExecInitPartitionInfo(), then
I'd imagine we'd want to use the same solution everywhere.

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/825031be-942c-8c24-6163-13c27f217a3d%40lab.ntt.co.jp