(2018/05/17 23:19), Ashutosh Bapat wrote:
On Thu, May 17, 2018 at 4:50 PM, Etsuro Fujita
<fujita.ets...@lab.ntt.co.jp> wrote:
(2018/05/16 22:49), Ashutosh Bapat wrote:
On Wed, May 16, 2018 at 4:01 PM, Etsuro Fujita
<fujita.ets...@lab.ntt.co.jp> wrote:
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.
Sorry, I don't understand this. Could you show me such an example?
Even without inheritance we can induce a ConvertRowtypeExpr on a base
relation which is not "other" relation
create table parent (a int, b int);
create table child () inherits(parent);
alter table child add constraint whole_par_const check ((child::parent).a = 1);
There is no way to tell by looking at the parsed query whether
pull_var_clause() from StoreRelCheck() will encounter a
ConvertRowtypeExpr or not. We could check whether the tables involved
are inherited or not, but that's more expensive than
is_converted_whole_row_reference().
Yeah, ISTM that the inheritance test makes things worse.
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.
Sounds more invasive. Considering where we are in the development cycle for
PG11, I think it'd be better to address this by something like the approach
I proposed. Anyway, +1 for asking the committer's opinion.
Thanks.
Let's ask that.
- On patch 0001-Handle-ConvertRowtypeExprs-in-pull_vars_clause.patch:
+extern bool
+is_converted_whole_row_reference(Node *node)
I think we should remove "extern" from the definition.
Done.
- On patch 0003-postgres_fdw-child-join-with-ConvertRowtypeExprs-cau.patch:
I think we can fix this by adding another flag to indicate whether we
deparsed the first live column of the relation, as in the "first" bool flag
in deparseTargetList.
Thanks. Fixed.
Thanks for updating the patch set! Other than pull_var_clause things,
the updated version looks good to me, so I'll mark this as Ready for
Committer. As I said before, I think this issue should be addressed in
advance of the PG11 release.
Best regards,
Etsuro Fujita
