postgres_fdw behaves oddly

While working on [1], I've found that postgres_fdw behaves oddly:

postgres=# create foreign table ft (a int) server loopback options
(table_name 't');
CREATE FOREIGN TABLE
postgres=# select tableoid, * from ft;
tableoid | a
----------+---
16400 | 1
(1 row)

postgres=# select tableoid, * from ft where tableoid = 16400;
tableoid | a
----------+---
(0 rows)

I think that this is because (a) the qual that contains tableoid can be
sent to the remote as shown in the EXPLAIN output:

postgres=# explain verbose select tableoid, * from ft where tableoid =
16400;
QUERY PLAN
----------------------------------------------------------------------
Foreign Scan on public.ft (cost=100.00..193.20 rows=2560 width=8)
Output: tableoid, a
Remote SQL: SELECT a FROM public.t WHERE ((tableoid = 16400::oid))
Planning time: 0.110 ms
(4 rows)

and because (b) the tableoid value can be differs between the local and
the remote. I think that one simple way of fixing such issues would be
to consider unsafe to send to the remote a qual that contains any system
columns (though we should probably give special treatment to quals
containing only ctid). With the modification of postgres_fdw, we get
the right result:

postgres=# select tableoid, * from ft where tableoid = 16400;
tableoid | a
----------+---
16400 | 1
(1 row)

However, it's not complete enough. Here is another surising result
(note no tableoid column in the select list):

postgres=# select * from ft where tableoid = 16400;
a
---
(0 rows)

I think that this is because create_foreignscan_plan doesn't refer to
rel->baserestrictinfo when detecting whether any system columns are
requested. By the additional modification of create_foreignscan_plan,
we get the right result:

postgres=# select * from ft where tableoid = 16400;
a
---
1
(1 row)

I'd also like to propose to change the function so as to make reference
to rel->reltargetlist, not to attr_needed, to match the code with other
places. Please find attached a patch.

Thanks,

[1] https://commitfest.postgresql.org/action/patch_view?id=1386

Best regards,
Etsuro Fujita

(2014/09/01 20:15), Etsuro Fujita wrote:
> While working on [1], I've found that postgres_fdw behaves oddly:
>
> postgres=# create foreign table ft (a int) server loopback options
> (table_name 't');
> CREATE FOREIGN TABLE
> postgres=# select tableoid, * from ft;
> tableoid | a
> ----------+---
> 16400 | 1
> (1 row)
>
> postgres=# select tableoid, * from ft where tableoid = 16400;
> tableoid | a
> ----------+---
> (0 rows)

> I think that one simple way of fixing such issues would be
> to consider unsafe to send to the remote a qual that contains any system
> columns.

I noticed the previous patch has overdone it. Attached is an updated
version of the patch.

Thanks,

PS:
> [1] https://commitfest.postgresql.org/action/patch_view?id=1386

I'll update the patch in [1] on top of this version.

Best regards,
Etsuro Fujita

(2014/09/02 18:55), Etsuro Fujita wrote:
> (2014/09/01 20:15), Etsuro Fujita wrote:
>> While working on [1], I've found that postgres_fdw behaves oddly:

I've revised the patch a bit further. Please find attached a patch.

Thanks,

Best regards,
Etsuro Fujita

Uh, where are we on this patch?

---------------------------------------------------------------------------

On Mon, Sep 8, 2014 at 09:30:32PM +0900, Etsuro Fujita wrote:
> (2014/09/02 18:55), Etsuro Fujita wrote:
> > (2014/09/01 20:15), Etsuro Fujita wrote:
> >> While working on [1], I've found that postgres_fdw behaves oddly:
>
> I've revised the patch a bit further. Please find attached a patch.
>
> Thanks,
>
> Best regards,
> Etsuro Fujita

> *** a/contrib/postgres_fdw/deparse.c
> --- b/contrib/postgres_fdw/deparse.c
> ***************
> *** 252,257 **** foreign_expr_walker(Node *node,
> --- 252,265 ----
> if (var->varno == glob_cxt->foreignrel->relid &&
> var->varlevelsup == 0)
> {
> + /*
> + * System columns can't be sent to remote.
> + *
> + * XXX: we should probably send ctid to remote.
> + */
> + if (var->varattno < 0)
> + return false;
> +
> /* Var belongs to foreign table */
> collation = var->varcollid;
> state = OidIsValid(collation) ? FDW_COLLATE_SAFE : FDW_COLLATE_NONE;
> ***************
> *** 1261,1266 **** deparseVar(Var *node, deparse_expr_cxt *context)
> --- 1269,1279 ----
> if (node->varno == context->foreignrel->relid &&
> node->varlevelsup == 0)
> {
> + /*
> + * System columns shouldn't be examined here.
> + */
> + Assert(node->varattno >= 0);
> +
> /* Var belongs to foreign table */
> deparseColumnRef(buf, node->varno, node->varattno, context->root);
> }
> *** a/src/backend/optimizer/plan/createplan.c
> --- b/src/backend/optimizer/plan/createplan.c
> ***************
> *** 20,25 ****
> --- 20,26 ----
> #include <math.h>
>
> #include "access/skey.h"
> + #include "access/sysattr.h"
> #include "catalog/pg_class.h"
> #include "foreign/fdwapi.h"
> #include "miscadmin.h"
> ***************
> *** 1945,1950 **** create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
> --- 1946,1953 ----
> RelOptInfo *rel = best_path->path.parent;
> Index scan_relid = rel->relid;
> RangeTblEntry *rte;
> + Bitmapset *attrs_used = NULL;
> + ListCell *lc;
> int i;
>
> /* it should be a base rel... */
> ***************
> *** 1993,2008 **** create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
> * bit of a kluge and might go away someday, so we intentionally leave it
> * out of the API presented to FDWs.
> */
> scan_plan->fsSystemCol = false;
> for (i = rel->min_attr; i < 0; i++)
> {
> ! if (!bms_is_empty(rel->attr_needed[i - rel->min_attr]))
> {
> scan_plan->fsSystemCol = true;
> break;
> }
> }
>
> return scan_plan;
> }
>
> --- 1996,2030 ----
> * bit of a kluge and might go away someday, so we intentionally leave it
> * out of the API presented to FDWs.
> */
> +
> + /*
> + * Add all the attributes needed for joins or final output. Note: we must
> + * look at reltargetlist, not the attr_needed data, because attr_needed
> + * isn't computed for inheritance child rels.
> + */
> + pull_varattnos((Node *) rel->reltargetlist, rel->relid, &attrs_used);
> +
> + /* Add all the attributes used by restriction clauses. */
> + foreach(lc, rel->baserestrictinfo)
> + {
> + RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
> +
> + pull_varattnos((Node *) rinfo->clause, rel->relid, &attrs_used);
> + }
> +
> + /* Are any system columns requested from rel? */
> scan_plan->fsSystemCol = false;
> for (i = rel->min_attr; i < 0; i++)
> {
> ! if (bms_is_member(i - FirstLowInvalidHeapAttributeNumber, attrs_used))
> {
> scan_plan->fsSystemCol = true;
> break;
> }
> }
>
> + bms_free(attrs_used);
> +
> return scan_plan;
> }
>

>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On Mon, Oct 13, 2014 at 3:30 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Uh, where are we on this patch?

Probably it should be added to the upcoming CF.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

(2014/10/14 8:53), Robert Haas wrote:
> On Mon, Oct 13, 2014 at 3:30 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>> Uh, where are we on this patch?
>
> Probably it should be added to the upcoming CF.

Done.

https://commitfest.postgresql.org/action/patch_view?id=1599

Thanks,

Best regards,
Etsuro Fujita

Since two separate issues 1. using reltargetlist instead of attr_needed and
2. system columns usage in FDW are being tackled here, we should separate
the patch into two one for each of the issues.

While I agree that the system columns shouldn't be sent to the remote node,
it doesn't look clear to me as to what would they or their values mean in
the context of foreign data. Some columns like tableoid would have a value
which is the OID of the foreign table, other system columns like
xmin/xmax/ctid will have different meanings (or no meaning) depending upon
the foreign data source. In case of later columns, each FDW would have its
own way of defining that meaning (I guess). But in any case, I agree that
we shouldn't send the system columns to the remote side.

Is there a way we can enforce this rule across all the FDWs? OR we want to
tackle it separately per FDW?

On Tue, Oct 14, 2014 at 8:05 AM, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
wrote:

> (2014/10/14 8:53), Robert Haas wrote:
>
>> On Mon, Oct 13, 2014 at 3:30 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>>
>>> Uh, where are we on this patch?
>>>
>>
>> Probably it should be added to the upcoming CF.
>>
>
> Done.
>
> https://commitfest.postgresql.org/action/patch_view?id=1599
>
> Thanks,
>
> Best regards,
> Etsuro Fujita
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Hi Ashutosh,

Thanks for the review!

(2014/11/10 20:05), Ashutosh Bapat wrote:
> Since two separate issues 1. using reltargetlist instead of attr_needed
> and 2. system columns usage in FDW are being tackled here, we should
> separate the patch into two one for each of the issues.

Agreed. Will split the patch into two.

> While I agree that the system columns shouldn't be sent to the remote
> node, it doesn't look clear to me as to what would they or their values
> mean in the context of foreign data. Some columns like tableoid would
> have a value which is the OID of the foreign table, other system columns
> like xmin/xmax/ctid will have different meanings (or no meaning)
> depending upon the foreign data source. In case of later columns, each
> FDW would have its own way of defining that meaning (I guess). But in
> any case, I agree that we shouldn't send the system columns to the
> remote side.
>
> Is there a way we can enforce this rule across all the FDWs? OR we want
> to tackle it separately per FDW?

ISTM it'd be better to tackle it separately because I feel that such an
enforcement by the PG core would go too far. I'm also concerned about a
possibility of impedance mismatching between such an enforcement and
postgres_fdw, which sends the ctid column to the remote side internally
when executing UPDATE/DELETE on foreign tables. See
postgresPlanForeignModify and eg, deparseUpdateSql.

Best regards,
Etsuro Fujita

(2014/11/11 18:45), Etsuro Fujita wrote:
> (2014/11/10 20:05), Ashutosh Bapat wrote:
>> Since two separate issues 1. using reltargetlist instead of attr_needed
>> and 2. system columns usage in FDW are being tackled here, we should
>> separate the patch into two one for each of the issues.
>
> Agreed. Will split the patch into two.

Here are splitted patches:

fscan-reltargetlist.patch - patch for #1
postgres_fdw-syscol.patch - patch for #2

Thanks,

Best regards,
Etsuro Fujita

Hi Fujita-san,

Here are my comments about the patch fscan_reltargetlist.patch
Sanity
--------
Patch applies and compiles cleanly.
Regressions in test/regress folder and postgres_fdw and file_fdw are clean.

1. This isn't your change, but we might be able to get rid of assignment
2071 /* Are any system columns requested from rel? */
2072 scan_plan->fsSystemCol = false;

since make_foreignscan() already does that. But I will leave upto you to
remove this assignment or not.

2. Instead of using rel->reltargetlist, we should use the tlist passed by
caller. This is the tlist expected from the Plan node. For foreign scans it
will be same as rel->reltargetlist. Using tlist would make the function
consistent with create_*scan_plan functions.

Otherwise, the patch looks good.

On Wed, Nov 12, 2014 at 12:55 PM, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp
> wrote:

> (2014/11/11 18:45), Etsuro Fujita wrote:
>
>> (2014/11/10 20:05), Ashutosh Bapat wrote:
>>
>>> Since two separate issues 1. using reltargetlist instead of attr_needed
>>> and 2. system columns usage in FDW are being tackled here, we should
>>> separate the patch into two one for each of the issues.
>>>
>>
>> Agreed. Will split the patch into two.
>>
>
> Here are splitted patches:
>
> fscan-reltargetlist.patch - patch for #1
> postgres_fdw-syscol.patch - patch for #2
>
>
> Thanks,
>
> Best regards,
> Etsuro Fujita
>

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Hi Fujita-san,

Here are comments for postgres_fdw-syscol patch.

Sanity
--------
The patch applies and compiles cleanly.
The server regression and regression in contrib/postgres_fdw,file_fdw run
cleanly.

Code
-------
1. Instead of a single liner comment "System columns can't be sent to
remote.", it might be better to explain why system columns can't be sent to
the remote.
2. The comment in deparseVar is single line comment, so it should start and
end on the same line i.e. /* and */ should be on the same line.
3. Since there is already a testcase which triggered this particular
change, it will good, if we add that to regression in postgres_fdw.

On Mon, Nov 17, 2014 at 4:06 PM, Ashutosh Bapat <
ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:

> Hi Fujita-san,
>
> Here are my comments about the patch fscan_reltargetlist.patch
> Sanity
> --------
> Patch applies and compiles cleanly.
> Regressions in test/regress folder and postgres_fdw and file_fdw are clean.
>
> 1. This isn't your change, but we might be able to get rid of assignment
> 2071 /* Are any system columns requested from rel? */
> 2072 scan_plan->fsSystemCol = false;
>
> since make_foreignscan() already does that. But I will leave upto you to
> remove this assignment or not.
>
> 2. Instead of using rel->reltargetlist, we should use the tlist passed by
> caller. This is the tlist expected from the Plan node. For foreign scans it
> will be same as rel->reltargetlist. Using tlist would make the function
> consistent with create_*scan_plan functions.
>
> Otherwise, the patch looks good.
>
> On Wed, Nov 12, 2014 at 12:55 PM, Etsuro Fujita <
> fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>
>> (2014/11/11 18:45), Etsuro Fujita wrote:
>>
>>> (2014/11/10 20:05), Ashutosh Bapat wrote:
>>>
>>>> Since two separate issues 1. using reltargetlist instead of attr_needed
>>>> and 2. system columns usage in FDW are being tackled here, we should
>>>> separate the patch into two one for each of the issues.
>>>>
>>>
>>> Agreed. Will split the patch into two.
>>>
>>
>> Here are splitted patches:
>>
>> fscan-reltargetlist.patch - patch for #1
>> postgres_fdw-syscol.patch - patch for #2
>>
>>
>> Thanks,
>>
>> Best regards,
>> Etsuro Fujita
>>
>
>
>
> --
> Best Wishes,
> Ashutosh Bapat
> EnterpriseDB Corporation
> The Postgres Database Company
>

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

(2014/11/17 19:36), Ashutosh Bapat wrote:
> Here are my comments about the patch fscan_reltargetlist.patch

Thanks for the review!

> 1. This isn't your change, but we might be able to get rid of assignment
> 2071 /* Are any system columns requested from rel? */
> 2072 scan_plan->fsSystemCol = false;
>
> since make_foreignscan() already does that. But I will leave upto you to
> remove this assignment or not.

As you pointed out, the assignment is redundant, but I think that that'd
improve the clarity and readability. So, I'd vote for leaving that as is.

> 2. Instead of using rel->reltargetlist, we should use the tlist passed
> by caller. This is the tlist expected from the Plan node. For foreign
> scans it will be same as rel->reltargetlist. Using tlist would make the
> function consistent with create_*scan_plan functions.

I disagree with that for the reasons mentioned below:

* For a foreign scan, tlist is *not* necessarily the same as
rel->reltargetlist (ie, there is a case where tlist contains all user
attributes while rel->reltargetlist contains only attributes actually
needed by the query). In such a case it'd be inefficient to use tlist
rather than rel->reltargetlist.

* I think that it'd improve the readability to match the code with other
places that execute similar processing, such as check_index_only() and
remove_unused_subquery_outputs().

Thanks,

Best regards,
Etsuro Fujita

(2014/11/17 19:54), Ashutosh Bapat wrote:
> Here are comments for postgres_fdw-syscol patch.

Thanks for the review!

> Code
> -------
> 1. Instead of a single liner comment "System columns can't be sent to
> remote.", it might be better to explain why system columns can't be sent
> to the remote.

Done.

> 2. The comment in deparseVar is single line comment, so it should start
> and end on the same line i.e. /* and */ should be on the same line.

Done.

> 3. Since there is already a testcase which triggered this particular
> change, it will good, if we add that to regression in postgres_fdw.

Done.

Please find attached an updated version of the patch.

Thanks,

Best regards,
Etsuro Fujita

On Tue, Nov 18, 2014 at 1:50 PM, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
wrote:

> (2014/11/17 19:36), Ashutosh Bapat wrote:
>
>> Here are my comments about the patch fscan_reltargetlist.patch
>>
>
> Thanks for the review!
>
> 1. This isn't your change, but we might be able to get rid of assignment
>> 2071 /* Are any system columns requested from rel? */
>> 2072 scan_plan->fsSystemCol = false;
>>
>> since make_foreignscan() already does that. But I will leave upto you to
>> remove this assignment or not.
>>
>
> As you pointed out, the assignment is redundant, but I think that that'd
> improve the clarity and readability. So, I'd vote for leaving that as is.
>

Ok. No problem.

>
> 2. Instead of using rel->reltargetlist, we should use the tlist passed
>> by caller. This is the tlist expected from the Plan node. For foreign
>> scans it will be same as rel->reltargetlist. Using tlist would make the
>> function consistent with create_*scan_plan functions.
>>
>
> I disagree with that for the reasons mentioned below:
>
> * For a foreign scan, tlist is *not* necessarily the same as
> rel->reltargetlist (ie, there is a case where tlist contains all user
> attributes while rel->reltargetlist contains only attributes actually
> needed by the query). In such a case it'd be inefficient to use tlist
> rather than rel->reltargetlist.
>

create_foreignscan_plan() is called from create_scan_plan(), which passes
the tlist. The later uses function use_physical_tlist() to decide which
targetlist should be used (physical or actual). As per code below in this
function

485 /*
486 * We can do this for real relation scans, subquery scans,
function scans,
487 * values scans, and CTE scans (but not for, eg, joins).
488 */
489 if (rel->rtekind != RTE_RELATION &&
490 rel->rtekind != RTE_SUBQUERY &&
491 rel->rtekind != RTE_FUNCTION &&
492 rel->rtekind != RTE_VALUES &&
493 rel->rtekind != RTE_CTE)
494 return false;
495
496 /*
497 * Can't do it with inheritance cases either (mainly because Append
498 * doesn't project).
499 */
500 if (rel->reloptkind != RELOPT_BASEREL)
501 return false;

For foreign tables as well as the tables under inheritance hierarchy it
uses the actual targetlist, which contains only the required columns IOW
rel->reltargetlist (see build_path_tlist()) with nested loop parameters
substituted. So, it never included unnecessary columns in the targetlist.
If we use rel->reltargetlist directly, without substituting nested loop
parameters, pull_var* coming across PlaceHolderVars or Vars from any
LATERAL or OUTER references. That won't create surprised now, but might do
that later. Second point is, the tlist passed to create_foreignscan_plan is
the one which gets projected during execution, so if we do not use it, we
might end up recording attributes different from the ones actually
projected by the node or required by quals.

>
> * I think that it'd improve the readability to match the code with other
> places that execute similar processing, such as check_index_only() and
> remove_unused_subquery_outputs().
>
>
Those functions are used during the path creation, so they don't have the
luxury of having ready-made tlist, hence use reltargetlist. But while in
planner, a ready-made tlist is available, so it better be used like all
create_*scan_plan() functions.

>
> Thanks,
>
> Best regards,
> Etsuro Fujita
>

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On Tue, Nov 18, 2014 at 1:55 PM, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
wrote:

> (2014/11/17 19:54), Ashutosh Bapat wrote:
>
>> Here are comments for postgres_fdw-syscol patch.
>>
>
> Thanks for the review!
>
> Code
>> -------
>> 1. Instead of a single liner comment "System columns can't be sent to
>> remote.", it might be better to explain why system columns can't be sent
>> to the remote.
>>
>
> Done.
>
>
I would add " and foreign values do not make sense locally (except may be
ctid clubbed with foreign server_id)" to make it more clear. But I will
leave that for the commiter to decide.

> 2. The comment in deparseVar is single line comment, so it should start
>> and end on the same line i.e. /* and */ should be on the same line.
>>
>
> Done.
>

Thanks

>
> 3. Since there is already a testcase which triggered this particular
>> change, it will good, if we add that to regression in postgres_fdw.
>>
>
> Done.
>
>
I think, a better case would be SELECT * FROM ft1 t1, pg_class t2 WHERE
t1.tableoid = t2.oid. The condition makes sure that the tableoid in the row
is same as the OID of the foreign table recorded in pg_class locally. And
the EXPLAIN of the query which clearly shows that the tableoid column in
not fetched from the foreign server.

Please find attached an updated version of the patch.

Once we resolve the other patch on this thread, I think this item can be
marked as ready for commiter from my side.

>
>
> Thanks,
>
> Best regards,
> Etsuro Fujita
>

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

(2014/11/18 18:27), Ashutosh Bapat wrote:
> On Tue, Nov 18, 2014 at 1:50 PM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp <mailto:fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>> wrote:
> (2014/11/17 19:36), Ashutosh Bapat wrote:

> Here are my comments about the patch fscan_reltargetlist.patch

> 2. Instead of using rel->reltargetlist, we should use the tlist
> passed
> by caller. This is the tlist expected from the Plan node. For
> foreign
> scans it will be same as rel->reltargetlist. Using tlist would
> make the
> function consistent with create_*scan_plan functions.

> I disagree with that for the reasons mentioned below:
>
> * For a foreign scan, tlist is *not* necessarily the same as
> rel->reltargetlist (ie, there is a case where tlist contains all
> user attributes while rel->reltargetlist contains only attributes
> actually needed by the query). In such a case it'd be inefficient
> to use tlist rather than rel->reltargetlist.

> create_foreignscan_plan() is called from create_scan_plan(), which
> passes the tlist. The later uses function use_physical_tlist() to decide
> which targetlist should be used (physical or actual). As per code below
> in this function
>
> 485 /*
> 486 * We can do this for real relation scans, subquery scans,
> function scans,
> 487 * values scans, and CTE scans (but not for, eg, joins).
> 488 */
> 489 if (rel->rtekind != RTE_RELATION &&
> 490 rel->rtekind != RTE_SUBQUERY &&
> 491 rel->rtekind != RTE_FUNCTION &&
> 492 rel->rtekind != RTE_VALUES &&
> 493 rel->rtekind != RTE_CTE)
> 494 return false;
> 495
> 496 /*
> 497 * Can't do it with inheritance cases either (mainly because
> Append
> 498 * doesn't project).
> 499 */
> 500 if (rel->reloptkind != RELOPT_BASEREL)
> 501 return false;
>
> For foreign tables as well as the tables under inheritance hierarchy it
> uses the actual targetlist, which contains only the required columns IOW
> rel->reltargetlist (see build_path_tlist()) with nested loop parameters
> substituted. So, it never included unnecessary columns in the
> targetlist.

Maybe I'm missing something, but I think you are overlooking the case
for foreign tables that are *not* under an inheritance hierarchy. (Note
that the rtekind for foreign tables is RTE_RELATION.)

Thanks,

Best regards,
Etsuro Fujita

(2014/11/18 18:37), Ashutosh Bapat wrote:
> On Tue, Nov 18, 2014 at 1:55 PM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp <mailto:fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>> wrote:
> (2014/11/17 19:54), Ashutosh Bapat wrote:

> Here are comments for postgres_fdw-syscol patch.

> Code
> -------
> 1. Instead of a single liner comment "System columns can't be
> sent to
> remote.", it might be better to explain why system columns can't
> be sent
> to the remote.

> Done.

> I would add " and foreign values do not make sense locally (except may
> be ctid clubbed with foreign server_id)" to make it more clear. But I
> will leave that for the commiter to decide.

OK.

> 3. Since there is already a testcase which triggered this particular
> change, it will good, if we add that to regression in postgres_fdw.

> Done.

> I think, a better case would be SELECT * FROM ft1 t1, pg_class t2 WHERE
> t1.tableoid = t2.oid. The condition makes sure that the tableoid in the
> row is same as the OID of the foreign table recorded in pg_class
> locally. And the EXPLAIN of the query which clearly shows that the
> tableoid column in not fetched from the foreign server.

I thought that test, but I didn't use it because I think we can't see
(at least from the EXPLAIN) why the qual is not pushed down: the qual
isn't pushed down possibly becasue the qual is considered as a *join*
qual, not because the qual just cotains tableoid. (Having said that, I
think we can see if the qual isn't pushed down as a join qual for a
parameterized plan, but ISTM it's worth complicating regression tests.)

> Once we resolve the other patch on this thread, I think this item can
> be marked as ready for commiter from my side.

OK. Thank you for the review.

Best regards,
Etsuro Fujita

On Wed, Nov 19, 2014 at 11:18 AM, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp
> wrote:

> (2014/11/18 18:27), Ashutosh Bapat wrote:
>
>> On Tue, Nov 18, 2014 at 1:50 PM, Etsuro Fujita
>> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp <mailto:fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>> wrote:
>> (2014/11/17 19:36), Ashutosh Bapat wrote:
>>
>
> Here are my comments about the patch fscan_reltargetlist.patch
>>
>
> 2. Instead of using rel->reltargetlist, we should use the tlist
>> passed
>> by caller. This is the tlist expected from the Plan node. For
>> foreign
>> scans it will be same as rel->reltargetlist. Using tlist would
>> make the
>> function consistent with create_*scan_plan functions.
>>
>
> I disagree with that for the reasons mentioned below:
>>
>> * For a foreign scan, tlist is *not* necessarily the same as
>> rel->reltargetlist (ie, there is a case where tlist contains all
>> user attributes while rel->reltargetlist contains only attributes
>> actually needed by the query). In such a case it'd be inefficient
>> to use tlist rather than rel->reltargetlist.
>>
>
> create_foreignscan_plan() is called from create_scan_plan(), which
>> passes the tlist. The later uses function use_physical_tlist() to decide
>> which targetlist should be used (physical or actual). As per code below
>> in this function
>>
>> 485 /*
>> 486 * We can do this for real relation scans, subquery scans,
>> function scans,
>> 487 * values scans, and CTE scans (but not for, eg, joins).
>> 488 */
>> 489 if (rel->rtekind != RTE_RELATION &&
>> 490 rel->rtekind != RTE_SUBQUERY &&
>> 491 rel->rtekind != RTE_FUNCTION &&
>> 492 rel->rtekind != RTE_VALUES &&
>> 493 rel->rtekind != RTE_CTE)
>> 494 return false;
>> 495
>> 496 /*
>> 497 * Can't do it with inheritance cases either (mainly because
>> Append
>> 498 * doesn't project).
>> 499 */
>> 500 if (rel->reloptkind != RELOPT_BASEREL)
>> 501 return false;
>>
>> For foreign tables as well as the tables under inheritance hierarchy it
>> uses the actual targetlist, which contains only the required columns IOW
>> rel->reltargetlist (see build_path_tlist()) with nested loop parameters
>> substituted. So, it never included unnecessary columns in the
>> targetlist.
>>
>
> Maybe I'm missing something, but I think you are overlooking the case for
> foreign tables that are *not* under an inheritance hierarchy. (Note that
> the rtekind for foreign tables is RTE_RELATION.)
>
>
>
Ah! you are right. I confused between rtekind and relkind. Sorry for that.
May be we should modify use_physical_tlist() to return false in case of
RELKIND_FOREIGN_TABLE, so that we can use tlist in
create_foreignscan_plan(). I do not see any create_*_plan() function using
reltargetlist directly.

> Thanks,
>
> Best regards,
> Etsuro Fujita
>

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

(2014/11/19 14:58), Ashutosh Bapat wrote:
> On Wed, Nov 19, 2014 at 11:18 AM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp <mailto:fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>> wrote:
> (2014/11/18 18:27), Ashutosh Bapat wrote:
> On Tue, Nov 18, 2014 at 1:50 PM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp
> (2014/11/17 19:36), Ashutosh Bapat wrote:

> Here are my comments about the patch
> fscan_reltargetlist.patch

> 2. Instead of using rel->reltargetlist, we should use
> the tlist
> passed
> by caller. This is the tlist expected from the Plan
> node. For
> foreign
> scans it will be same as rel->reltargetlist. Using
> tlist would
> make the
> function consistent with create_*scan_plan functions.

> I disagree with that for the reasons mentioned below:
>
> * For a foreign scan, tlist is *not* necessarily the same as
> rel->reltargetlist (ie, there is a case where tlist
> contains all
> user attributes while rel->reltargetlist contains only
> attributes
> actually needed by the query). In such a case it'd be
> inefficient
> to use tlist rather than rel->reltargetlist.

> create_foreignscan_plan() is called from create_scan_plan(), which
> passes the tlist. The later uses function use_physical_tlist()
> to decide
> which targetlist should be used (physical or actual). As per
> code below
> in this function
>
> 485 /*
> 486 * We can do this for real relation scans, subquery
> scans,
> function scans,
> 487 * values scans, and CTE scans (but not for, eg, joins).
> 488 */
> 489 if (rel->rtekind != RTE_RELATION &&
> 490 rel->rtekind != RTE_SUBQUERY &&
> 491 rel->rtekind != RTE_FUNCTION &&
> 492 rel->rtekind != RTE_VALUES &&
> 493 rel->rtekind != RTE_CTE)
> 494 return false;
> 495
> 496 /*
> 497 * Can't do it with inheritance cases either (mainly
> because
> Append
> 498 * doesn't project).
> 499 */
> 500 if (rel->reloptkind != RELOPT_BASEREL)
> 501 return false;
>
> For foreign tables as well as the tables under inheritance
> hierarchy it
> uses the actual targetlist, which contains only the required
> columns IOW
> rel->reltargetlist (see build_path_tlist()) with nested loop
> parameters
> substituted. So, it never included unnecessary columns in the
> targetlist.

> Maybe I'm missing something, but I think you are overlooking the
> case for foreign tables that are *not* under an inheritance
> hierarchy. (Note that the rtekind for foreign tables is RTE_RELATION.)

> Ah! you are right. I confused between rtekind and relkind. Sorry for
> that. May be we should modify use_physical_tlist() to return false in
> case of RELKIND_FOREIGN_TABLE, so that we can use tlist in
> create_foreignscan_plan(). I do not see any create_*_plan() function
> using reltargetlist directly.

Yeah, I think we can do that, but I'm not sure that we should use tlist
in create_foreignscan_plan(), not rel->reltargetlist. How about leaving
this for committers to decide.

Thanks,

Best regards,
Etsuro Fujita

On Wed, Nov 19, 2014 at 12:14 PM, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp
> wrote:

> (2014/11/19 14:58), Ashutosh Bapat wrote:
>
>> On Wed, Nov 19, 2014 at 11:18 AM, Etsuro Fujita
>> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp <mailto:fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>> wrote:
>> (2014/11/18 18:27), Ashutosh Bapat wrote:
>> On Tue, Nov 18, 2014 at 1:50 PM, Etsuro Fujita
>> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp
>> (2014/11/17 19:36), Ashutosh Bapat wrote:
>>
>
> Here are my comments about the patch
>> fscan_reltargetlist.patch
>>
>
> 2. Instead of using rel->reltargetlist, we should use
>> the tlist
>> passed
>> by caller. This is the tlist expected from the Plan
>> node. For
>> foreign
>> scans it will be same as rel->reltargetlist. Using
>> tlist would
>> make the
>> function consistent with create_*scan_plan functions.
>>
>
> I disagree with that for the reasons mentioned below:
>>
>> * For a foreign scan, tlist is *not* necessarily the same as
>> rel->reltargetlist (ie, there is a case where tlist
>> contains all
>> user attributes while rel->reltargetlist contains only
>> attributes
>> actually needed by the query). In such a case it'd be
>> inefficient
>> to use tlist rather than rel->reltargetlist.
>>
>
> create_foreignscan_plan() is called from create_scan_plan(), which
>> passes the tlist. The later uses function use_physical_tlist()
>> to decide
>> which targetlist should be used (physical or actual). As per
>> code below
>> in this function
>>
>> 485 /*
>> 486 * We can do this for real relation scans, subquery
>> scans,
>> function scans,
>> 487 * values scans, and CTE scans (but not for, eg,
>> joins).
>> 488 */
>> 489 if (rel->rtekind != RTE_RELATION &&
>> 490 rel->rtekind != RTE_SUBQUERY &&
>> 491 rel->rtekind != RTE_FUNCTION &&
>> 492 rel->rtekind != RTE_VALUES &&
>> 493 rel->rtekind != RTE_CTE)
>> 494 return false;
>> 495
>> 496 /*
>> 497 * Can't do it with inheritance cases either (mainly
>> because
>> Append
>> 498 * doesn't project).
>> 499 */
>> 500 if (rel->reloptkind != RELOPT_BASEREL)
>> 501 return false;
>>
>> For foreign tables as well as the tables under inheritance
>> hierarchy it
>> uses the actual targetlist, which contains only the required
>> columns IOW
>> rel->reltargetlist (see build_path_tlist()) with nested loop
>> parameters
>> substituted. So, it never included unnecessary columns in the
>> targetlist.
>>
>
> Maybe I'm missing something, but I think you are overlooking the
>> case for foreign tables that are *not* under an inheritance
>> hierarchy. (Note that the rtekind for foreign tables is
>> RTE_RELATION.)
>>
>
> Ah! you are right. I confused between rtekind and relkind. Sorry for
>> that. May be we should modify use_physical_tlist() to return false in
>> case of RELKIND_FOREIGN_TABLE, so that we can use tlist in
>> create_foreignscan_plan(). I do not see any create_*_plan() function
>> using reltargetlist directly.
>>
>
> Yeah, I think we can do that, but I'm not sure that we should use tlist in
> create_foreignscan_plan(), not rel->reltargetlist. How about leaving this
> for committers to decide.
>
>
I am fine with that. May be you want to add an XXX comment there to bring
it to the committer's notice.

>
> Thanks,
>
> Best regards,
> Etsuro Fujita
>

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

(2014/11/19 14:55), Etsuro Fujita wrote:
> (2014/11/18 18:37), Ashutosh Bapat wrote:
>> On Tue, Nov 18, 2014 at 1:55 PM, Etsuro Fujita
>> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp <mailto:fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>> wrote:
>> (2014/11/17 19:54), Ashutosh Bapat wrote:
>
>> Here are comments for postgres_fdw-syscol patch.

>> 3. Since there is already a testcase which triggered this
>> particular
>> change, it will good, if we add that to regression in
>> postgres_fdw.
>
>> Done.
>
>> I think, a better case would be SELECT * FROM ft1 t1, pg_class t2 WHERE
>> t1.tableoid = t2.oid. The condition makes sure that the tableoid in the
>> row is same as the OID of the foreign table recorded in pg_class
>> locally. And the EXPLAIN of the query which clearly shows that the
>> tableoid column in not fetched from the foreign server.
>
> I thought that test, but I didn't use it because I think we can't see
> (at least from the EXPLAIN) why the qual is not pushed down: the qual
> isn't pushed down possibly becasue the qual is considered as a *join*
> qual, not because the qual just cotains tableoid. (Having said that, I
> think we can see if the qual isn't pushed down as a join qual for a
> parameterized plan, but ISTM it's worth complicating regression tests.)

Sorry, I incorrectly wrote the last sentence. What I tried to write is,
ISTM it's *not* worth complicating regression tests.

Best regards,
Etsuro Fujita

(2014/11/19 15:56), Ashutosh Bapat wrote:
> On Wed, Nov 19, 2014 at 12:14 PM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp <mailto:fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>> wrote:
> (2014/11/19 14:58), Ashutosh Bapat wrote:

> May be we should modify use_physical_tlist() to return
> false in
> case of RELKIND_FOREIGN_TABLE, so that we can use tlist in
> create_foreignscan_plan(). I do not see any create_*_plan() function
> using reltargetlist directly.

> Yeah, I think we can do that, but I'm not sure that we should use
> tlist in create_foreignscan_plan(), not rel->reltargetlist. How
> about leaving this for committers to decide.

> I am fine with that. May be you want to add an XXX comment there to
> bring it to the committer's notice.

It's ok, but I'm not convinced with your idea. So, I think the comment
can be adequately described by you, not by me. So, my proposal is for
you to add the comment to the CF app. Could you do that?

Thanks,

Best regards,
Etsuro Fujita

Ok. I added that comment to the commitfest and changed the status to "ready
for commiter".

On Wed, Nov 19, 2014 at 1:10 PM, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
wrote:

> (2014/11/19 15:56), Ashutosh Bapat wrote:
>
>> On Wed, Nov 19, 2014 at 12:14 PM, Etsuro Fujita
>> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp <mailto:fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>> wrote:
>> (2014/11/19 14:58), Ashutosh Bapat wrote:
>>
>
> May be we should modify use_physical_tlist() to return
>> false in
>> case of RELKIND_FOREIGN_TABLE, so that we can use tlist in
>> create_foreignscan_plan(). I do not see any create_*_plan()
>> function
>> using reltargetlist directly.
>>
>
> Yeah, I think we can do that, but I'm not sure that we should use
>> tlist in create_foreignscan_plan(), not rel->reltargetlist. How
>> about leaving this for committers to decide.
>>
>
> I am fine with that. May be you want to add an XXX comment there to
>> bring it to the committer's notice.
>>
>
> It's ok, but I'm not convinced with your idea. So, I think the comment
> can be adequately described by you, not by me. So, my proposal is for you
> to add the comment to the CF app. Could you do that?
>
>
> Thanks,
>
> Best regards,
> Etsuro Fujita
>

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

(2014/11/19 18:21), Ashutosh Bapat wrote:
> Ok. I added that comment to the commitfest and changed the status to
> "ready for commiter".

Many thanks!

PS: the link to the review emails that discuss the issue doesn't work
properly, so I re-added the link.

Best regards,
Etsuro Fujita

Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> writes:
> (2014/11/19 18:21), Ashutosh Bapat wrote:
>> Ok. I added that comment to the commitfest and changed the status to
>> "ready for commiter".

> Many thanks!

I committed this with some cosmetic adjustments, and one not-so-cosmetic
one: I left it continuing to allow CTID conditions to be sent to the
remote server. That works today and we shouldn't disable it, especially
not in the back branches where it could mean a performance regression
in a minor release.

As for the question of whether reltargetlist or tlist should be examined,
reltargetlist is the correct thing. I do not see a need to modify
use_physical_tlist either. If you trace through what happens in e.g.
postgres_fdw, you'll notice that it only bothers to retrieve actually-used
columns (which it computes correctly) from the remote side. It then
constructs a scan tuple that corresponds to the foreign table's physical
column set, inserting NULLs for any unfetched columns. This is handed
back to execScan.c and matches what a heapscan or indexscan would produce.
The point of the use_physical_tlist optimization is to avoid a projection
step *on this tuple* if we don't really need to do one (ie, if it'd be
just as good for the scan node's output tuple to be identical to the row's
physical tuple). If we disabled use_physical_tlist then we'd be forcing
a projection step that would have the effect of removing the NULLs for
unfetched columns, but it would not actually be of value, just as it isn't
in the corresponding cases for heap/index scans.

BTW, given that there wasn't any very good way to test the createplan.c
fix except by adding test cases to postgres_fdw, I didn't see much value
in splitting the patch in two. The committed patch includes tests that
the createplan.c bug is fixed as well as the postgres_fdw one.

regards, tom lane

On Sun, Nov 23, 2014 at 2:46 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> writes:
> > (2014/11/19 18:21), Ashutosh Bapat wrote:
> >> Ok. I added that comment to the commitfest and changed the status to
> >> "ready for commiter".
>
> > Many thanks!
>
> I committed this with some cosmetic adjustments, and one not-so-cosmetic
> one: I left it continuing to allow CTID conditions to be sent to the
> remote server. That works today and we shouldn't disable it, especially
> not in the back branches where it could mean a performance regression
> in a minor release.
>
>
Thanks.

> As for the question of whether reltargetlist or tlist should be examined,
> reltargetlist is the correct thing. I do not see a need to modify
> use_physical_tlist either. If you trace through what happens in e.g.
> postgres_fdw, you'll notice that it only bothers to retrieve actually-used
> columns (which it computes correctly) from the remote side. It then
> constructs a scan tuple that corresponds to the foreign table's physical
> column set, inserting NULLs for any unfetched columns. This is handed
> back to execScan.c and matches what a heapscan or indexscan would produce.
> The point of the use_physical_tlist optimization is to avoid a projection
> step *on this tuple* if we don't really need to do one (ie, if it'd be
> just as good for the scan node's output tuple to be identical to the row's
> physical tuple). If we disabled use_physical_tlist then we'd be forcing
> a projection step that would have the effect of removing the NULLs for
> unfetched columns, but it would not actually be of value, just as it isn't
> in the corresponding cases for heap/index scans.
>
> For the current patch, that's fair.

In grouping_planner() the targetlist of the topmost plan node of join tree
is changed to the set of expressions requested in the query.
1554 else
1555 {
1556 /*
1557 * Otherwise, just replace the subplan's flat
tlist with
1558 * the desired tlist.
1559 */
1560 result_plan->targetlist = sub_tlist;
1561 }
This means that for a simple SELECT from the foreign table, projection
would be necessary in ExecScan if all the columns are not required.

So, the strategy to emulate heap or index scan wastes extra cycles and
memory in adding NULL columns which are not required and then filtering
them during the projection (within ExecScan itself). It also limits the
ability of foreign scans to fetch shippable expressions when those
expressions are part of the targetlist and fetching their values (instead
of the columns participating in the expressions) reduces the size of the
fetched row.

These cases do not exist for heap scans or index scans because the tuples
are obtained directly from the buffer, so no extra memory required for the
extra columns and projection as well as expression evaluation is anyway
required.

BTW, given that there wasn't any very good way to test the createplan.c
> fix except by adding test cases to postgres_fdw, I didn't see much value
> in splitting the patch in two. The committed patch includes tests that
> the createplan.c bug is fixed as well as the postgres_fdw one.
>
> regards, tom lane
>

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

(2014/11/23 6:16), Tom Lane wrote:
> I committed this with some cosmetic adjustments, and one not-so-cosmetic
> one:

Thanks for improving and committing the patch!

Best regards,
Etsuro Fujita