Re: Expression errors with "FOR UPDATE" and postgres_fdw with partition wise join enabled.

On Wed, May 16, 2018 at 4:01 PM, Etsuro Fujita
<fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>
>>>> So we could
>>>> really minimize the code change and avoid the additional overhead caused
>>>> by
>>>> the is_converted_whole_row_reference test added to pull_var_clause. I
>>>> think
>>>> the latter would be rather important, because PWJ is disabled by
>>>> default.
>>>> What do you think about that approach?
>>>
>>>
>>> Partition-wise join and partition-wise aggregation are disabled by
>>> default temporarily. We will change it in near future once the memory
>>> consumption issue has been tackled. The features are useful and users
>>> would want those to be enabled by default like other query
>>> optimizations. So, we can't take a decision based on that.
>
>
> Yeah, I really agree on that point! However, considering that
> pull_var_clause is used in many places where partitioning is *not* involved,
> I still think it's better to avoid spending extra cycles needed only for
> partitioning in that function.

Right. The changes done in inheritance_planner() imply that we can
encounter a ConvertRowtypeExpr even in the expressions for a relation
which is not a child relation in the translated query. It was a child
in the original query but after translating it becomes a parent and
has ConvertRowtypeExpr somewhere there. So, there is no way to know
whether a given expression will have ConvertRowtypeExpr in it. That's
my understanding. But if we can device such a test, I am happy to
apply that test before or witin the call to pull_var_clause().

We don't need that expensive test if user has passed
PVC_RECURSE_CONVERTROWTYPE. So we could test that first and then check
the shape of expression tree. It would cause more asymmetry in
pull_var_clause(), but we can live with it or change the order of
tests for all the three options. I will differ that to a committer.

> + /* Construct the conversion map. */
> + parent_desc = lookup_rowtype_tupdesc(cre->resulttype, 0);
>
> I think it'd be better to pass -1, not 0, as the typmod argument for that
> function because that would be consistent with other places where we use
> that function with named rowtypes (eg, ruleutils.c).

Done.

>
> -is_subquery_var(Var *node, RelOptInfo *foreignrel, int *relno, int *colno)
> +is_subquery_column(Node *node, Index varno, RelOptInfo *foreignrel,
> + int *relno, int *colno)
>
> -1 for that change; because 1) we use "var" for implying many things as in
> eg, pull_var_clause and 2) I think it'd make back-patching easy to keep the
> name as-is.

I think pull_var_clause is the only place where we do that and I don't
like that very much. I also don't like is_subquery_var() name since
it's too specific. We might want that kind of functionality to ship
generic expressions and not just Vars in future. Usually we would be
searching for columns in the subquery targetlist so the name change
looks good to me. IIRC, the function was added one release back, so
backpatching pain, if any, won't be much. But I don't think we should
carry a misnomer for many releases to come. Let's differ this to a
committer.

Here's set of updated patches.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company