Re: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)
Hi Fujita-san,
I reviewed the v4 patch, and here are some comments from me.
* After applying this patch, pull_varattnos() should not called in
unnecessary places. For developers who want list of
columns-to-be-processed for some purpose, it would be nice to mention
when they should use pull_varattnos() and when they should not, maybe
at the comments of pull_varattnos() implementation.
* deparseReturningList() and postgresGetForeignRelSize() in
contrib/postgres_fdw/ also call pull_varattnos() to determine which
column to be in the SELECT clause of remote query. Shouldn't these be
replaced in the same manner? Other pull_varattnos() calls are for
restrictions, so IIUC they can't be replaced.
* Through this review I thought up that lazy evaluation approach might
fit attr_needed. I mean that we leave attr_needed for child relations
empty, and fill it at the first request for it. This would avoid
useless computation of attr_needed for child relations, though this
idea has been considered but thrown away before...
2014-08-20 18:55 GMT+09:00 Etsuro Fujita fujita.ets...@lab.ntt.co.jp:
Hi Ashutish,
(2014/08/14 22:30), Ashutosh Bapat wrote:
On Thu, Aug 14, 2014 at 10:05 AM, Etsuro Fujita
fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote:
(2014/08/08 18:51), Etsuro Fujita wrote:
(2014/06/30 22:48), Tom Lane wrote:
I wonder whether it isn't time to change that. It was coded
like that
originally only because calculating the values would've been a
waste of
cycles at the time. But this is at least the third place
where it'd be
useful to have attr_needed for child rels.
I've revised the patch.
There was a problem with the previous patch, which will be described
below. Attached is the updated version of the patch addressing that.
Here are some more comments
Thank you for the further review!
attr_needed also has the attributes used in the restriction clauses as
seen in distribute_qual_to_rels(), so, it looks unnecessary to call
pull_varattnos() on the clauses in baserestrictinfo in functions
check_selective_binary_conversion(), remove_unused_subquery_outputs(),
check_index_only().
IIUC, I think it's *necessary* to do that in those functions since the
attributes used in the restriction clauses in baserestrictinfo are not added
to attr_needed due the following code in distribute_qual_to_rels.
/*
* If it's a join clause (either naturally, or because delayed by
* outer-join rules), add vars used in the clause to targetlists of
their
* relations, so that they will be emitted by the plan nodes that scan
* those relations (else they won't be available at the join node!).
*
* Note: if the clause gets absorbed into an EquivalenceClass then this
* may be unnecessary, but for now we have to do it to cover the case
* where the EC becomes ec_broken and we end up reinserting the original
* clauses into the plan.
*/
if (bms_membership(relids) == BMS_MULTIPLE)
{
List *vars = pull_var_clause(clause,
PVC_RECURSE_AGGREGATES,
PVC_INCLUDE_PLACEHOLDERS);
add_vars_to_targetlist(root, vars, relids, false);
list_free(vars);
}
Although in case of RTE_RELATION, the 0th entry in attr_needed
corresponds to FirstLowInvalidHeapAttributeNumber + 1, it's always safer
to use it is RelOptInfo::min_attr, in case get_relation_info() wants to
change assumption or somewhere down the line some other part of code
wants to change attr_needed information. It may be unlikely, that it
would change, but it will be better to stick to comments in RelOptInfo
443 AttrNumber min_attr; /* smallest attrno of rel (often
0) */
444 AttrNumber max_attr; /* largest attrno of rel */
445 Relids *attr_needed;/* array indexed [min_attr ..
max_attr] */
Good point! Attached is the revised version of the patch.
Thanks,
Best regards,
Etsuro Fujita
--
Shigeru HANADA
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)
Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes:
[ attr_needed-v4.patch ]
I looked this over, and TBH I'm rather disappointed. The patch adds
150 lines of dubiously-correct code in order to save ... uh, well,
actually it *adds* code, because the places that are supposedly getting
a benefit are changed like this:
*** 799,806 check_selective_binary_conversion(RelOptInfo *baserel,
}
/* Collect all the attributes needed for joins or final output. */
! pull_varattnos((Node *) baserel-reltargetlist, baserel-relid,
! attrs_used);
/* Add all the attributes used by restriction clauses. */
foreach(lc, baserel-baserestrictinfo)
--- 799,810
}
/* Collect all the attributes needed for joins or final output. */
! for (i = baserel-min_attr; i = baserel-max_attr; i++)
! {
! if (!bms_is_empty(baserel-attr_needed[i - baserel-min_attr]))
! attrs_used = bms_add_member(attrs_used,
!
i - FirstLowInvalidHeapAttributeNumber);
! }
/* Add all the attributes used by restriction clauses. */
foreach(lc, baserel-baserestrictinfo)
That's not simpler, it's not easier to understand, and it's probably not
faster either. We could address some of those complaints by encapsulating
the above loop into a utility function, but still, I come away with the
feeling that it's not worth changing this. Considering that all the
places that are doing this then proceed to use pull_varattnos to add on
attnos from the restriction clauses, it seems like using pull_varattnos
on the reltargetlist isn't such a bad thing after all.
So I'm inclined to reject this. It seemed like a good idea in the
abstract, but the concrete result isn't very attractive, and doesn't
seem like an improvement over what we have.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)
(2014/08/27 3:27), Tom Lane wrote: Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes: [ attr_needed-v4.patch ] I looked this over, and TBH I'm rather disappointed. The patch adds 150 lines of dubiously-correct code in order to save ... uh, well, Just for my study, could you tell me why you think that the code is dubiously-correct? Considering that all the places that are doing this then proceed to use pull_varattnos to add on attnos from the restriction clauses, it seems like using pull_varattnos on the reltargetlist isn't such a bad thing after all. I agree with you on that point. So I'm inclined to reject this. It seemed like a good idea in the abstract, but the concrete result isn't very attractive, and doesn't seem like an improvement over what we have. Okay. I'll withdraw the patch. Thank you for taking the time to review the patch! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)
Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes: (2014/08/27 3:27), Tom Lane wrote: I looked this over, and TBH I'm rather disappointed. The patch adds 150 lines of dubiously-correct code in order to save ... uh, well, Just for my study, could you tell me why you think that the code is dubiously-correct? It might be fine; I did not actually review the new adjust_appendrel_attr_needed code in any detail. What's scaring me off it is (1) it's a lot longer and more complicated than I'd thought it would be, and (2) you already made several bug fixes in it, which is often an indicator that additional problems lurk. It's possible there's some other, simpler, way to compute child attr_needed arrays that would resolve (1) and (2). However, even if we had a simple and obviously-correct way to do that, it still seems like there's not very much benefit to be had after all. So my thought that this would be worth doing seems wrong, and I must apologize to you for sending you chasing down a dead end :-( regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)
(2014/08/27 11:06), Tom Lane wrote: Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes: (2014/08/27 3:27), Tom Lane wrote: I looked this over, and TBH I'm rather disappointed. The patch adds 150 lines of dubiously-correct code in order to save ... uh, well, Just for my study, could you tell me why you think that the code is dubiously-correct? It might be fine; I did not actually review the new adjust_appendrel_attr_needed code in any detail. What's scaring me off it is (1) it's a lot longer and more complicated than I'd thought it would be, and (2) you already made several bug fixes in it, which is often an indicator that additional problems lurk. Okay. It's possible there's some other, simpler, way to compute child attr_needed arrays that would resolve (1) and (2). However, even if we had a simple and obviously-correct way to do that, it still seems like there's not very much benefit to be had after all. So my thought that this would be worth doing seems wrong, and I must apologize to you for sending you chasing down a dead end :-( Please don't worry yourself about that! Thanks, Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)
(2014/08/21 13:21), Ashutosh Bapat wrote: On Wed, Aug 20, 2014 at 3:25 PM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote: Hi Ashutish, I am sorry that I mistook your name's spelling. (2014/08/14 22:30), Ashutosh Bapat wrote: On Thu, Aug 14, 2014 at 10:05 AM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.__co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote: (2014/08/08 18:51), Etsuro Fujita wrote: (2014/06/30 22:48), Tom Lane wrote: I wonder whether it isn't time to change that. It was coded like that originally only because calculating the values would've been a waste of cycles at the time. But this is at least the third place where it'd be useful to have attr_needed for child rels. There was a problem with the previous patch, which will be described below. Attached is the updated version of the patch addressing that. Here are some more comments attr_needed also has the attributes used in the restriction clauses as seen in distribute_qual_to_rels(), so, it looks unnecessary to call pull_varattnos() on the clauses in baserestrictinfo in functions check_selective_binary___conversion(), remove_unused_subquery___outputs(), check_index_only(). IIUC, I think it's *necessary* to do that in those functions since the attributes used in the restriction clauses in baserestrictinfo are not added to attr_needed due the following code in distribute_qual_to_rels. That's right. Thanks for pointing that out. Although in case of RTE_RELATION, the 0th entry in attr_needed corresponds to FirstLowInvalidHeapAttributeNu__mber + 1, it's always safer to use it is RelOptInfo::min_attr, in case get_relation_info() wants to change assumption or somewhere down the line some other part of code wants to change attr_needed information. It may be unlikely, that it would change, but it will be better to stick to comments in RelOptInfo 443 AttrNumber min_attr; /* smallest attrno of rel (often 0) */ 444 AttrNumber max_attr; /* largest attrno of rel */ 445 Relids *attr_needed;/* array indexed [min_attr .. max_attr] */ Good point! Attached is the revised version of the patch. If the patch is not in the commit-fest, can you please add it there? I've already done that: https://commitfest.postgresql.org/action/patch_view?id=1529 From my side, the review is done, it should be marked ready for committer, unless somebody else wants to review. Many thanks! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)
On Thu, Aug 21, 2014 at 3:00 PM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp wrote: (2014/08/21 13:21), Ashutosh Bapat wrote: On Wed, Aug 20, 2014 at 3:25 PM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote: Hi Ashutish, I am sorry that I mistook your name's spelling. (2014/08/14 22:30), Ashutosh Bapat wrote: On Thu, Aug 14, 2014 at 10:05 AM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.__co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote: (2014/08/08 18:51), Etsuro Fujita wrote: (2014/06/30 22:48), Tom Lane wrote: I wonder whether it isn't time to change that. It was coded like that originally only because calculating the values would've been a waste of cycles at the time. But this is at least the third place where it'd be useful to have attr_needed for child rels. There was a problem with the previous patch, which will be described below. Attached is the updated version of the patch addressing that. Here are some more comments attr_needed also has the attributes used in the restriction clauses as seen in distribute_qual_to_rels(), so, it looks unnecessary to call pull_varattnos() on the clauses in baserestrictinfo in functions check_selective_binary___conversion(), remove_unused_subquery___outputs(), check_index_only(). IIUC, I think it's *necessary* to do that in those functions since the attributes used in the restriction clauses in baserestrictinfo are not added to attr_needed due the following code in distribute_qual_to_rels. That's right. Thanks for pointing that out. Although in case of RTE_RELATION, the 0th entry in attr_needed corresponds to FirstLowInvalidHeapAttributeNu__mber + 1, it's always safer to use it is RelOptInfo::min_attr, in case get_relation_info() wants to change assumption or somewhere down the line some other part of code wants to change attr_needed information. It may be unlikely, that it would change, but it will be better to stick to comments in RelOptInfo 443 AttrNumber min_attr; /* smallest attrno of rel (often 0) */ 444 AttrNumber max_attr; /* largest attrno of rel */ 445 Relids *attr_needed;/* array indexed [min_attr .. max_attr] */ Good point! Attached is the revised version of the patch. If the patch is not in the commit-fest, can you please add it there? I've already done that: https://commitfest.postgresql.org/action/patch_view?id=1529 From my side, the review is done, it should be marked ready for committer, unless somebody else wants to review. Many thanks! Thanks. Since, I haven't seen anybody else commenting here and I do not have any further comments to make, I have marked it as ready for committer. Best regards, Etsuro Fujita -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Re: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)
Hi Ashutish,
(2014/08/14 22:30), Ashutosh Bapat wrote:
On Thu, Aug 14, 2014 at 10:05 AM, Etsuro Fujita
fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote:
(2014/08/08 18:51), Etsuro Fujita wrote:
(2014/06/30 22:48), Tom Lane wrote:
I wonder whether it isn't time to change that. It was coded
like that
originally only because calculating the values would've been a
waste of
cycles at the time. But this is at least the third place
where it'd be
useful to have attr_needed for child rels.
I've revised the patch.
There was a problem with the previous patch, which will be described
below. Attached is the updated version of the patch addressing that.
Here are some more comments
Thank you for the further review!
attr_needed also has the attributes used in the restriction clauses as
seen in distribute_qual_to_rels(), so, it looks unnecessary to call
pull_varattnos() on the clauses in baserestrictinfo in functions
check_selective_binary_conversion(), remove_unused_subquery_outputs(),
check_index_only().
IIUC, I think it's *necessary* to do that in those functions since the
attributes used in the restriction clauses in baserestrictinfo are not
added to attr_needed due the following code in distribute_qual_to_rels.
/*
* If it's a join clause (either naturally, or because delayed by
* outer-join rules), add vars used in the clause to targetlists of
their
* relations, so that they will be emitted by the plan nodes that scan
* those relations (else they won't be available at the join node!).
*
* Note: if the clause gets absorbed into an EquivalenceClass then this
* may be unnecessary, but for now we have to do it to cover the case
* where the EC becomes ec_broken and we end up reinserting the
original
* clauses into the plan.
*/
if (bms_membership(relids) == BMS_MULTIPLE)
{
List *vars = pull_var_clause(clause,
PVC_RECURSE_AGGREGATES,
PVC_INCLUDE_PLACEHOLDERS);
add_vars_to_targetlist(root, vars, relids, false);
list_free(vars);
}
Although in case of RTE_RELATION, the 0th entry in attr_needed
corresponds to FirstLowInvalidHeapAttributeNumber + 1, it's always safer
to use it is RelOptInfo::min_attr, in case get_relation_info() wants to
change assumption or somewhere down the line some other part of code
wants to change attr_needed information. It may be unlikely, that it
would change, but it will be better to stick to comments in RelOptInfo
443 AttrNumber min_attr; /* smallest attrno of rel (often
0) */
444 AttrNumber max_attr; /* largest attrno of rel */
445 Relids *attr_needed;/* array indexed [min_attr ..
max_attr] */
Good point! Attached is the revised version of the patch.
Thanks,
Best regards,
Etsuro Fujita
*** a/contrib/file_fdw/file_fdw.c
--- b/contrib/file_fdw/file_fdw.c
***
*** 799,806 check_selective_binary_conversion(RelOptInfo *baserel,
}
/* Collect all the attributes needed for joins or final output. */
! pull_varattnos((Node *) baserel-reltargetlist, baserel-relid,
! attrs_used);
/* Add all the attributes used by restriction clauses. */
foreach(lc, baserel-baserestrictinfo)
--- 799,810
}
/* Collect all the attributes needed for joins or final output. */
! for (i = baserel-min_attr; i = baserel-max_attr; i++)
! {
! if (!bms_is_empty(baserel-attr_needed[i - baserel-min_attr]))
! attrs_used = bms_add_member(attrs_used,
! i - FirstLowInvalidHeapAttributeNumber);
! }
/* Add all the attributes used by restriction clauses. */
foreach(lc, baserel-baserestrictinfo)
*** a/src/backend/optimizer/path/allpaths.c
--- b/src/backend/optimizer/path/allpaths.c
***
*** 577,588 set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
childrel-has_eclass_joins = rel-has_eclass_joins;
/*
! * Note: we could compute appropriate attr_needed data for the child's
! * variables, by transforming the parent's attr_needed through the
! * translated_vars mapping. However, currently there's no need
! * because attr_needed is only examined for base relations not
! * otherrels. So we just leave the child's attr_needed empty.
*/
/*
* Compute the child's size.
--- 577,585
childrel-has_eclass_joins = rel-has_eclass_joins;
/*
! * Compute the child's attr_needed.
*/
+ adjust_appendrel_attr_needed(rel, childrel, appinfo);
/*
* Compute the child's size.
***
*** 2173,2178 remove_unused_subquery_outputs(Query *subquery, RelOptInfo *rel)
--- 2170,2176
{
Bitmapset *attrs_used = NULL;
ListCell *lc;
+ int i;
/*
* Do nothing if subquery has UNION/INTERSECT/EXCEPT: in principle we
***
***
Re: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)
On Wed, Aug 20, 2014 at 3:25 PM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp
wrote:
Hi Ashutish,
(2014/08/14 22:30), Ashutosh Bapat wrote:
On Thu, Aug 14, 2014 at 10:05 AM, Etsuro Fujita
fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote:
(2014/08/08 18:51), Etsuro Fujita wrote:
(2014/06/30 22:48), Tom Lane wrote:
I wonder whether it isn't time to change that. It was coded
like that
originally only because calculating the values would've been a
waste of
cycles at the time. But this is at least the third place
where it'd be
useful to have attr_needed for child rels.
I've revised the patch.
There was a problem with the previous patch, which will be described
below. Attached is the updated version of the patch addressing that.
Here are some more comments
Thank you for the further review!
attr_needed also has the attributes used in the restriction clauses as
seen in distribute_qual_to_rels(), so, it looks unnecessary to call
pull_varattnos() on the clauses in baserestrictinfo in functions
check_selective_binary_conversion(), remove_unused_subquery_outputs(),
check_index_only().
IIUC, I think it's *necessary* to do that in those functions since the
attributes used in the restriction clauses in baserestrictinfo are not
added to attr_needed due the following code in distribute_qual_to_rels.
That's right. Thanks for pointing that out.
/*
* If it's a join clause (either naturally, or because delayed by
* outer-join rules), add vars used in the clause to targetlists of
their
* relations, so that they will be emitted by the plan nodes that scan
* those relations (else they won't be available at the join node!).
*
* Note: if the clause gets absorbed into an EquivalenceClass then this
* may be unnecessary, but for now we have to do it to cover the case
* where the EC becomes ec_broken and we end up reinserting the
original
* clauses into the plan.
*/
if (bms_membership(relids) == BMS_MULTIPLE)
{
List *vars = pull_var_clause(clause,
PVC_RECURSE_AGGREGATES,
PVC_INCLUDE_PLACEHOLDERS);
add_vars_to_targetlist(root, vars, relids, false);
list_free(vars);
}
Although in case of RTE_RELATION, the 0th entry in attr_needed
corresponds to FirstLowInvalidHeapAttributeNumber + 1, it's always safer
to use it is RelOptInfo::min_attr, in case get_relation_info() wants to
change assumption or somewhere down the line some other part of code
wants to change attr_needed information. It may be unlikely, that it
would change, but it will be better to stick to comments in RelOptInfo
443 AttrNumber min_attr; /* smallest attrno of rel (often
0) */
444 AttrNumber max_attr; /* largest attrno of rel */
445 Relids *attr_needed;/* array indexed [min_attr ..
max_attr] */
Good point! Attached is the revised version of the patch.
If the patch is not in the commit-fest, can you please add it there? From
my side, the review is done, it should be marked ready for committer,
unless somebody else wants to review.
Thanks,
Best regards,
Etsuro Fujita
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
Re: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)
Hi, On Thu, Aug 14, 2014 at 10:05 AM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp wrote: (2014/08/08 18:51), Etsuro Fujita wrote: (2014/06/30 22:48), Tom Lane wrote: I wonder whether it isn't time to change that. It was coded like that originally only because calculating the values would've been a waste of cycles at the time. But this is at least the third place where it'd be useful to have attr_needed for child rels. I've revised the patch. There was a problem with the previous patch, which will be described below. Attached is the updated version of the patch addressing that. The previous patch doesn't cope with some UNION ALL cases properly. So, e.g., the server will crash for the following query: postgres=# create table ta1 (f1 int); CREATE TABLE postgres=# create table ta2 (f2 int primary key, f3 int); CREATE TABLE postgres=# create table tb1 (f1 int); CREATE TABLE postgres=# create table tb2 (f2 int primary key, f3 int); CREATE TABLE postgres=# explain verbose select f1 from ((select f1, f2 from (select f1, f2, f3 from ta1 left join ta2 on f1 = f2 limit 1) ssa) union all (select f1, f2 from (select f1, f2, f3 from tb1 left join tb2 on f1 = f2 limit 1) ssb)) ss; With the updated version, we get the right result: postgres=# explain verbose select f1 from ((select f1, f2 from (select f1, f2, f3 from ta1 left join ta2 on f1 = f2 limit 1) ssa) union all (select f1, f2 from (select f1, f2, f3 from tb1 left join tb2 on f1 = f2 limit 1) ssb)) ss; QUERY PLAN Append (cost=0.00..0.05 rows=2 width=4) - Subquery Scan on ssa (cost=0.00..0.02 rows=1 width=4) Output: ssa.f1 - Limit (cost=0.00..0.01 rows=1 width=4) Output: ta1.f1, (NULL::integer), (NULL::integer) - Seq Scan on public.ta1 (cost=0.00..34.00 rows=2400 width=4) Output: ta1.f1, NULL::integer, NULL::integer - Subquery Scan on ssb (cost=0.00..0.02 rows=1 width=4) Output: ssb.f1 - Limit (cost=0.00..0.01 rows=1 width=4) Output: tb1.f1, (NULL::integer), (NULL::integer) - Seq Scan on public.tb1 (cost=0.00..34.00 rows=2400 width=4) Output: tb1.f1, NULL::integer, NULL::integer Planning time: 0.453 ms (14 rows) While thinking to address this problem, Ashutosh also expressed concern about the UNION ALL handling in the previous patch in a private email. Thank you for the review, Ashutosh! Thanks for taking care of this one. Here are some more comments attr_needed also has the attributes used in the restriction clauses as seen in distribute_qual_to_rels(), so, it looks unnecessary to call pull_varattnos() on the clauses in baserestrictinfo in functions check_selective_binary_conversion(), remove_unused_subquery_outputs(), check_index_only(). Although in case of RTE_RELATION, the 0th entry in attr_needed corresponds to FirstLowInvalidHeapAttributeNumber + 1, it's always safer to use it is RelOptInfo::min_attr, in case get_relation_info() wants to change assumption or somewhere down the line some other part of code wants to change attr_needed information. It may be unlikely, that it would change, but it will be better to stick to comments in RelOptInfo 443 AttrNumber min_attr; /* smallest attrno of rel (often 0) */ 444 AttrNumber max_attr; /* largest attrno of rel */ 445 Relids *attr_needed;/* array indexed [min_attr .. max_attr] */ Thanks, Best regards, Etsuro Fujita -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
