postgres_fdw : altering foreign table not invalidating prepare statement execution plan.

Hi,

Thanks for the report.

On 2016/04/04 15:17, Rajkumar Raghuwanshi wrote:

Hi,

I observed below in postgres_fdw

* .Observation: *Prepare statement execution plan is not getting changed
even after altering foreign table to point to new schema.

[ ... ]

PREPARE stmt_ft AS select c1,c2 from ft;

EXECUTE stmt_ft;
c1 | c2
----+-------
1 | s1.lt
(1 row)

--changed foreign table ft pointing schema from s1 to s2
ALTER foreign table ft options (SET schema_name 's2', SET table_name 'lt');
ANALYZE ft;

EXPLAIN (COSTS OFF, VERBOSE) EXECUTE stmt_ft;
QUERY PLAN
----------------------------------------
Foreign Scan on public.ft
Output: c1, c2
Remote SQL: SELECT c1, c2 FROM s1.lt
(3 rows)

I wonder if performing relcache invalidation upon ATExecGenericOptions()
is the correct solution for this problem. The attached patch fixes the
issue for me.

Thanks,
Amit

Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:

On 2016/04/04 15:17, Rajkumar Raghuwanshi wrote:

* .Observation: *Prepare statement execution plan is not getting changed
even after altering foreign table to point to new schema.

I wonder if performing relcache invalidation upon ATExecGenericOptions()
is the correct solution for this problem. The attached patch fixes the
issue for me.

A forced relcache inval will certainly fix it, but I'm not sure if that's
the best (or only) place to put it.

A related issue, now that I've seen this example, is that altering
FDW-level or server-level options won't cause a replan either. I'm
not sure there's any very good fix for that. Surely we don't want
to try to identify all tables belonging to the FDW or server and
issue relcache invals on all of them.

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

On Mon, Apr 4, 2016 at 11:24 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:

On 2016/04/04 15:17, Rajkumar Raghuwanshi wrote:

* .Observation: *Prepare statement execution plan is not getting changed
even after altering foreign table to point to new schema.

I wonder if performing relcache invalidation upon ATExecGenericOptions()
is the correct solution for this problem. The attached patch fixes the
issue for me.

A forced relcache inval will certainly fix it, but I'm not sure if that's
the best (or only) place to put it.

A related issue, now that I've seen this example, is that altering
FDW-level or server-level options won't cause a replan either. I'm
not sure there's any very good fix for that. Surely we don't want
to try to identify all tables belonging to the FDW or server and
issue relcache invals on all of them.

Hm, some kind of PlanInvalItem-based solution could work maybe? Or
some solution on lines of recent PlanCacheUserMappingCallback() added
by fbe5a3fb, wherein I see this comment which sounds like a similar
solution could be built for servers and FDWs:

+       /*
+        * If the plan has pushed down foreign joins, those join may become
+        * unsafe to push down because of user mapping changes. Invalidate only
+        * the generic plan, since changes to user mapping do not invalidate the
+        * parse tree.
+        */

Missing something am I?

Thanks,
Amit

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

Amit Langote <amitlangote09@gmail.com> writes:

On Mon, Apr 4, 2016 at 11:24 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

A related issue, now that I've seen this example, is that altering
FDW-level or server-level options won't cause a replan either. I'm
not sure there's any very good fix for that. Surely we don't want
to try to identify all tables belonging to the FDW or server and
issue relcache invals on all of them.

Hm, some kind of PlanInvalItem-based solution could work maybe?

Hm, so we'd expect that whenever an FDW consulted the options while
making a plan, it'd have to record a plan dependency on them? That
would be a clean fix maybe, but I'm worried that third-party FDWs
would fail to get the word about needing to do this.

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

At Mon, 04 Apr 2016 11:23:34 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in <9798.1459783414@sss.pgh.pa.us>

Amit Langote <amitlangote09@gmail.com> writes:

On Mon, Apr 4, 2016 at 11:24 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

A related issue, now that I've seen this example, is that altering
FDW-level or server-level options won't cause a replan either. I'm
not sure there's any very good fix for that. Surely we don't want
to try to identify all tables belonging to the FDW or server and
issue relcache invals on all of them.

Hm, some kind of PlanInvalItem-based solution could work maybe?

Hm, so we'd expect that whenever an FDW consulted the options while
making a plan, it'd have to record a plan dependency on them? That
would be a clean fix maybe, but I'm worried that third-party FDWs
would fail to get the word about needing to do this.

If the "recording" means recoding oids to CachedPlanSource like
relationOids, it seems to be able to be recorded automatically by
the core, not by fdw side, we already know the server id for
foreign tables.

I'm missing something?

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

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

On 2016/04/05 0:23, Tom Lane wrote:

Amit Langote <amitlangote09@gmail.com> writes:

On Mon, Apr 4, 2016 at 11:24 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

A related issue, now that I've seen this example, is that altering
FDW-level or server-level options won't cause a replan either. I'm
not sure there's any very good fix for that. Surely we don't want
to try to identify all tables belonging to the FDW or server and
issue relcache invals on all of them.

Hm, some kind of PlanInvalItem-based solution could work maybe?

Hm, so we'd expect that whenever an FDW consulted the options while
making a plan, it'd have to record a plan dependency on them? That
would be a clean fix maybe, but I'm worried that third-party FDWs
would fail to get the word about needing to do this.

I would imagine that that level of granularity may be a little too much; I
mean tracking dependencies at the level of individual FDW/foreign
table/foreign server options. I think it should really be possible to do
the entire thing in core instead of requiring this to be made a concern of
FDW authors. How about the attached that teaches
extract_query_dependencies() to add a foreign table and associated foreign
data wrapper and foreign server to invalItems. Also, it adds plan cache
callbacks for respective caches.

One thing that I observed that may not be all that surprising is that we
may need a similar mechanism for postgres_fdw's connection cache, which
doesn't drop connections using older server connection info after I alter
them. I was trying to test my patch by altering dbaname option of a
foreign server but that was silly, ;). Although, I did confirm that the
patch works by altering use_remote_estimates server option. I could not
really test for FDW options though.

Thoughts?

Thanks,
Amit

At Tue, 5 Apr 2016 14:24:52 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in <57034C24.1000203@lab.ntt.co.jp>

On 2016/04/05 0:23, Tom Lane wrote:

Amit Langote <amitlangote09@gmail.com> writes:

On Mon, Apr 4, 2016 at 11:24 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

A related issue, now that I've seen this example, is that altering
FDW-level or server-level options won't cause a replan either. I'm
not sure there's any very good fix for that. Surely we don't want
to try to identify all tables belonging to the FDW or server and
issue relcache invals on all of them.

Hm, some kind of PlanInvalItem-based solution could work maybe?

Hm, so we'd expect that whenever an FDW consulted the options while
making a plan, it'd have to record a plan dependency on them? That
would be a clean fix maybe, but I'm worried that third-party FDWs
would fail to get the word about needing to do this.

I would imagine that that level of granularity may be a little too much; I
mean tracking dependencies at the level of individual FDW/foreign
table/foreign server options. I think it should really be possible to do
the entire thing in core instead of requiring this to be made a concern of
FDW authors. How about the attached that teaches
extract_query_dependencies() to add a foreign table and associated foreign
data wrapper and foreign server to invalItems. Also, it adds plan cache
callbacks for respective caches.

It seems to work but some renaming of functions would needed.

One thing that I observed that may not be all that surprising is that we
may need a similar mechanism for postgres_fdw's connection cache, which
doesn't drop connections using older server connection info after I alter
them. I was trying to test my patch by altering dbaname option of a
foreign server but that was silly, ;). Although, I did confirm that the
patch works by altering use_remote_estimates server option. I could not
really test for FDW options though.

Thoughts?

It seems a issue of FDW drivers but notification can be issued by
the core. The attached ultra-POC patch does it.

Disconnecting of a fdw connection with active transaction causes
an unexpected rollback on the remote side. So disconnection
should be allowed only when xact_depth == 0 in
pgfdw_subxact_callback().

There are so many parameters that affect connections, which are
listed as PQconninfoOptions. It is a bit too complex to detect
changes of one or some of them. So, I try to use object access
hook even though using hook is not proper as fdw interface. An
additional interface would be needed to do this anyway.

With this patch, making any change on foreign servers or user
mappings corresponding to exiting connection causes
disconnection. This could be moved in to core, with the following
API like this.

reoutine->NotifyObjectModification(ObjectAccessType access,
Oid classId, Oid objId);

- using object access hook (which is put by the core itself) is not bad?

- If ok, the API above looks bad. Any better API?

By the way, AlterUserMapping seems missing calling
InvokeObjectPostAlterHook. Is this a bug?

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

On 2016/04/05 18:44, Kyotaro HORIGUCHI wrote:

At Tue, 5 Apr 2016 14:24:52 +0900, Amit Langote wrote:

On 2016/04/05 0:23, Tom Lane wrote:

Amit Langote <amitlangote09@gmail.com> writes:

Hm, some kind of PlanInvalItem-based solution could work maybe?

Hm, so we'd expect that whenever an FDW consulted the options while
making a plan, it'd have to record a plan dependency on them? That
would be a clean fix maybe, but I'm worried that third-party FDWs
would fail to get the word about needing to do this.

I would imagine that that level of granularity may be a little too much; I
mean tracking dependencies at the level of individual FDW/foreign
table/foreign server options. I think it should really be possible to do
the entire thing in core instead of requiring this to be made a concern of
FDW authors. How about the attached that teaches
extract_query_dependencies() to add a foreign table and associated foreign
data wrapper and foreign server to invalItems. Also, it adds plan cache
callbacks for respective caches.

It seems to work but some renaming of functions would needed.

Yeah, I felt the names were getting quite long, too :)

One thing that I observed that may not be all that surprising is that we
may need a similar mechanism for postgres_fdw's connection cache, which
doesn't drop connections using older server connection info after I alter
them. I was trying to test my patch by altering dbaname option of a
foreign server but that was silly, ;). Although, I did confirm that the
patch works by altering use_remote_estimates server option. I could not
really test for FDW options though.

Thoughts?

It seems a issue of FDW drivers but notification can be issued by
the core. The attached ultra-POC patch does it.

Disconnecting of a fdw connection with active transaction causes
an unexpected rollback on the remote side. So disconnection
should be allowed only when xact_depth == 0 in
pgfdw_subxact_callback().

There are so many parameters that affect connections, which are
listed as PQconninfoOptions. It is a bit too complex to detect
changes of one or some of them. So, I try to use object access
hook even though using hook is not proper as fdw interface. An
additional interface would be needed to do this anyway.

With this patch, making any change on foreign servers or user
mappings corresponding to exiting connection causes
disconnection. This could be moved in to core, with the following
API like this.

reoutine->NotifyObjectModification(ObjectAccessType access,
Oid classId, Oid objId);

- using object access hook (which is put by the core itself) is not bad?

- If ok, the API above looks bad. Any better API?

Although helps achieve our goal here, object access hook stuff seems to be
targeted at different users:

/*
* Hook on object accesses. This is intended as infrastructure for security
* and logging plugins.
*/
object_access_hook_type object_access_hook = NULL;

I just noticed the following comment above GetConnection():

*
* XXX Note that caching connections theoretically requires a mechanism to
* detect change of FDW objects to invalidate already established connections.
* We could manage that by watching for invalidation events on the relevant
* syscaches. For the moment, though, it's not clear that this would really
* be useful and not mere pedantry. We could not flush any active connections
* mid-transaction anyway.

So, the hazard of flushing the connection mid-transaction sounds like it
may be a show-stopper here, even if we research some approach based on
syscache invalidation. Although I see that your patch takes care of it.

By the way, AlterUserMapping seems missing calling
InvokeObjectPostAlterHook. Is this a bug?

Probably, yes.

Thanks,
Amit

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

Hi,

At Tue, 5 Apr 2016 19:46:04 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in <5703976C.30308@lab.ntt.co.jp>

On 2016/04/05 18:44, Kyotaro HORIGUCHI wrote:

At Tue, 5 Apr 2016 14:24:52 +0900, Amit Langote wrote:
With this patch, making any change on foreign servers or user
mappings corresponding to exiting connection causes
disconnection. This could be moved in to core, with the following
API like this.

reoutine->NotifyObjectModification(ObjectAccessType access,
Oid classId, Oid objId);

- using object access hook (which is put by the core itself) is not bad?

- If ok, the API above looks bad. Any better API?

Although helps achieve our goal here, object access hook stuff seems to be
targeted at different users:

/*
* Hook on object accesses. This is intended as infrastructure for security
* and logging plugins.
*/
object_access_hook_type object_access_hook = NULL;

Yeah, furthermore, it doesn't notify to other backends, that is
what I forgotten at the time:(

So, it seems reasonable that all stuffs ride on the cache
invalidation mechanism.

Object class id and object id should be necessary to be
adequitely notificated to fdws but the current invalidation
mechanism donesn't convey the latter. It is hidden in hash
code. This is resolved just by additional member in PlanInvalItem
or resolving oid from hash code(this would need catalog scan..).

PlanCache*Callback() just do invalidtion and throw the causeal
PlanInvalItem away. If plancache had oid lists of foreign
servers, foreign tables, user mappings or FDWS, we can notify
FDWs of invalidation with the causal object.

- Adding CachedPlanSource with some additional oid lists, which
will be built by extract_query_dependencies.

- In PlanCache*Callback(), class id and object id of the causal
object is notified to FDW.

I just noticed the following comment above GetConnection():

*
* XXX Note that caching connections theoretically requires a mechanism to
* detect change of FDW objects to invalidate already established connections.
* We could manage that by watching for invalidation events on the relevant
* syscaches. For the moment, though, it's not clear that this would really
* be useful and not mere pedantry. We could not flush any active connections
* mid-transaction anyway.

So, the hazard of flushing the connection mid-transaction sounds like it
may be a show-stopper here, even if we research some approach based on
syscache invalidation. Although I see that your patch takes care of it.

Yeah. Altough cache invalidation would accur amid transaction..

By the way, AlterUserMapping seems missing calling
InvokeObjectPostAlterHook. Is this a bug?

Probably, yes.

But we dont' see a proper way to "fix" this since we here don't
use this.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

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

On 2016/04/05 14:24, Amit Langote wrote:

On 2016/04/05 0:23, Tom Lane wrote:

Amit Langote <amitlangote09@gmail.com> writes:

Hm, some kind of PlanInvalItem-based solution could work maybe?

Hm, so we'd expect that whenever an FDW consulted the options while
making a plan, it'd have to record a plan dependency on them? That
would be a clean fix maybe, but I'm worried that third-party FDWs
would fail to get the word about needing to do this.

I would imagine that that level of granularity may be a little too much; I
mean tracking dependencies at the level of individual FDW/foreign
table/foreign server options. I think it should really be possible to do
the entire thing in core instead of requiring this to be made a concern of
FDW authors. How about the attached that teaches
extract_query_dependencies() to add a foreign table and associated foreign
data wrapper and foreign server to invalItems. Also, it adds plan cache
callbacks for respective caches.

One thing that I observed that may not be all that surprising is that we
may need a similar mechanism for postgres_fdw's connection cache, which
doesn't drop connections using older server connection info after I alter
them. I was trying to test my patch by altering dbaname option of a
foreign server but that was silly, ;). Although, I did confirm that the
patch works by altering use_remote_estimates server option. I could not
really test for FDW options though.

Thoughts?

I added this to next CF, just in case:
https://commitfest.postgresql.org/10/609/

Thanks,
Amit

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

On 2016/04/04 23:24, Tom Lane wrote:

Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:

On 2016/04/04 15:17, Rajkumar Raghuwanshi wrote:

* .Observation: *Prepare statement execution plan is not getting changed
even after altering foreign table to point to new schema.

I wonder if performing relcache invalidation upon ATExecGenericOptions()
is the correct solution for this problem. The attached patch fixes the
issue for me.

A forced relcache inval will certainly fix it, but I'm not sure if that's
the best (or only) place to put it.

A related issue, now that I've seen this example, is that altering
FDW-level or server-level options won't cause a replan either. I'm
not sure there's any very good fix for that. Surely we don't want
to try to identify all tables belonging to the FDW or server and
issue relcache invals on all of them.

While improving join pushdown in postgres_fdw, I happened to notice that
the fetch_size option in 9.6 has the same issue. A forced replan
discussed above would fix that issue, but I'm not sure that that's a
good idea, because the fetch_size option, which postgres_fdw gets at
GetForeignRelSize, is not used for anything in creating a plan. So, as
far as the fetch_size option is concerned, a better solution is (1) to
consult that option at execution time, probably at BeginForeignScan, not
at planning time such as GetForiegnRelSize (and (2) to not cause that
replan when altering that option.) Maybe I'm missing something, though.

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

On Tue, Apr 5, 2016 at 10:54 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

On 2016/04/05 0:23, Tom Lane wrote:

Amit Langote <amitlangote09@gmail.com> writes:

On Mon, Apr 4, 2016 at 11:24 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

A related issue, now that I've seen this example, is that altering
FDW-level or server-level options won't cause a replan either. I'm
not sure there's any very good fix for that. Surely we don't want
to try to identify all tables belonging to the FDW or server and
issue relcache invals on all of them.

Hm, some kind of PlanInvalItem-based solution could work maybe?

Hm, so we'd expect that whenever an FDW consulted the options while
making a plan, it'd have to record a plan dependency on them? That
would be a clean fix maybe, but I'm worried that third-party FDWs
would fail to get the word about needing to do this.

I would imagine that that level of granularity may be a little too much; I
mean tracking dependencies at the level of individual FDW/foreign
table/foreign server options. I think it should really be possible to do
the entire thing in core instead of requiring this to be made a concern of
FDW authors. How about the attached that teaches
extract_query_dependencies() to add a foreign table and associated foreign
data wrapper and foreign server to invalItems. Also, it adds plan cache
callbacks for respective caches.

Although this is a better solution than the previous one, it
invalidates the query tree along with the generic plan. Invalidating
the query tree and the generic plan required parsing the query again
and generating the plan. Instead I think, we should invalidate only
the generic plan and not the query tree. The attached patch adds the
dependencies from create_foreignscan_plan() which is called for any
foreign access. I have also added testcases to test the functionality.
Let me know your comments on the patch.

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

On Wed, Aug 24, 2016 at 1:55 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:

On 2016/04/04 23:24, Tom Lane wrote:

Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:

On 2016/04/04 15:17, Rajkumar Raghuwanshi wrote:

* .Observation: *Prepare statement execution plan is not getting changed
even after altering foreign table to point to new schema.

I wonder if performing relcache invalidation upon ATExecGenericOptions()
is the correct solution for this problem. The attached patch fixes the
issue for me.

A forced relcache inval will certainly fix it, but I'm not sure if that's
the best (or only) place to put it.

A related issue, now that I've seen this example, is that altering
FDW-level or server-level options won't cause a replan either. I'm
not sure there's any very good fix for that. Surely we don't want
to try to identify all tables belonging to the FDW or server and
issue relcache invals on all of them.

While improving join pushdown in postgres_fdw, I happened to notice that the
fetch_size option in 9.6 has the same issue. A forced replan discussed
above would fix that issue, but I'm not sure that that's a good idea,
because the fetch_size option, which postgres_fdw gets at GetForeignRelSize,
is not used for anything in creating a plan. So, as far as the fetch_size
option is concerned, a better solution is (1) to consult that option at
execution time, probably at BeginForeignScan, not at planning time such as
GetForiegnRelSize (and (2) to not cause that replan when altering that
option.) Maybe I'm missing something, though.

As explained in my latest patch, an FDW knows which of the options,
when changed, render a plan invalid. If we have to track the changes
for each option, an FDW will need to consulted to check whether that
options affects the plan or not. That may be an overkill and there is
high chance that the FDW authors wouldn't bother implementing that
hook. Let me know your views on my latest patch on this thread.

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

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

On 2016/09/29 20:06, Ashutosh Bapat wrote:

On Wed, Aug 24, 2016 at 1:55 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:

On 2016/04/04 23:24, Tom Lane wrote:

A related issue, now that I've seen this example, is that altering
FDW-level or server-level options won't cause a replan either. I'm
not sure there's any very good fix for that. Surely we don't want
to try to identify all tables belonging to the FDW or server and
issue relcache invals on all of them.

While improving join pushdown in postgres_fdw, I happened to notice that the
fetch_size option in 9.6 has the same issue. A forced replan discussed
above would fix that issue, but I'm not sure that that's a good idea,
because the fetch_size option, which postgres_fdw gets at GetForeignRelSize,
is not used for anything in creating a plan. So, as far as the fetch_size
option is concerned, a better solution is (1) to consult that option at
execution time, probably at BeginForeignScan, not at planning time such as
GetForiegnRelSize (and (2) to not cause that replan when altering that
option.) Maybe I'm missing something, though.

As explained in my latest patch, an FDW knows which of the options,
when changed, render a plan invalid. If we have to track the changes
for each option, an FDW will need to consulted to check whether that
options affects the plan or not. That may be an overkill and there is
high chance that the FDW authors wouldn't bother implementing that
hook.

I thought it would be better to track that dependencies to avoid useless
replans, but I changed my mind; I agree with Amit's approach. In
addition to what you mentioned, ISTM that users don't change such
options frequently, so I think Amit's approach is much practical.

Let me know your views on my latest patch on this thread.

OK, will do.

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

On 2016/09/29 20:03, Ashutosh Bapat wrote:

On Tue, Apr 5, 2016 at 10:54 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

How about the attached that teaches
extract_query_dependencies() to add a foreign table and associated foreign
data wrapper and foreign server to invalItems. Also, it adds plan cache
callbacks for respective caches.

Although this is a better solution than the previous one, it
invalidates the query tree along with the generic plan. Invalidating
the query tree and the generic plan required parsing the query again
and generating the plan. Instead I think, we should invalidate only
the generic plan and not the query tree.

Agreed.

The attached patch adds the
dependencies from create_foreignscan_plan() which is called for any
foreign access. I have also added testcases to test the functionality.
Let me know your comments on the patch.

Hmm. I'm not sure that that's a good idea.

I was thinking the changes to setrefs.c proposed by Amit to collect that
dependencies would be probably OK, but I wasn't sure that it's a good
idea that he used PlanCacheFuncCallback as the syscache inval callback
function for the foreign object caches because it invalidates not only
generic plans but query trees, as you mentioned downthread. So, I was
thinking to modify his patch so that we add a new syscache inval
callback function for the caches that is much like PlanCacheFuncCallback
but only invalidates generic plans.

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

The attached patch adds the
dependencies from create_foreignscan_plan() which is called for any
foreign access. I have also added testcases to test the functionality.
Let me know your comments on the patch.

Hmm. I'm not sure that that's a good idea.

I was thinking the changes to setrefs.c proposed by Amit to collect that
dependencies would be probably OK, but I wasn't sure that it's a good idea
that he used PlanCacheFuncCallback as the syscache inval callback function
for the foreign object caches because it invalidates not only generic plans
but query trees, as you mentioned downthread. So, I was thinking to modify
his patch so that we add a new syscache inval callback function for the
caches that is much like PlanCacheFuncCallback but only invalidates generic
plans.

PlanCacheFuncCallback() invalidates the query tree only when
invalItems are added to the plan source. The patch adds the
dependencies in root->glob->invalItems, which standard_planner()
copies into PlannedStmt::invalItems. This is then copied into the
gplan->stmt_list. Thus PlanCacheFuncCallback never invalidates the
query tree. I have verified this under the debugger. Am I missing
something?

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

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

On Fri, Sep 30, 2016 at 1:09 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:

The attached patch adds the
dependencies from create_foreignscan_plan() which is called for any
foreign access. I have also added testcases to test the functionality.
Let me know your comments on the patch.

Hmm. I'm not sure that that's a good idea.

I was thinking the changes to setrefs.c proposed by Amit to collect that
dependencies would be probably OK, but I wasn't sure that it's a good idea
that he used PlanCacheFuncCallback as the syscache inval callback function
for the foreign object caches because it invalidates not only generic plans
but query trees, as you mentioned downthread. So, I was thinking to modify
his patch so that we add a new syscache inval callback function for the
caches that is much like PlanCacheFuncCallback but only invalidates generic
plans.

PlanCacheFuncCallback() invalidates the query tree only when
invalItems are added to the plan source. The patch adds the
dependencies in root->glob->invalItems, which standard_planner()
copies into PlannedStmt::invalItems. This is then copied into the
gplan->stmt_list. Thus PlanCacheFuncCallback never invalidates the
query tree. I have verified this under the debugger. Am I missing
something?

Moved to next CF. Feel free to continue the work on this item.
--
Michael

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

On 2016/09/30 1:09, Ashutosh Bapat wrote:

You wrote:

The attached patch adds the
dependencies from create_foreignscan_plan() which is called for any
foreign access. I have also added testcases to test the functionality.
Let me know your comments on the patch.

I wrote:

Hmm. I'm not sure that that's a good idea.

I was thinking the changes to setrefs.c proposed by Amit to collect that
dependencies would be probably OK, but I wasn't sure that it's a good idea
that he used PlanCacheFuncCallback as the syscache inval callback function
for the foreign object caches because it invalidates not only generic plans
but query trees, as you mentioned downthread. So, I was thinking to modify
his patch so that we add a new syscache inval callback function for the
caches that is much like PlanCacheFuncCallback but only invalidates generic
plans.

PlanCacheFuncCallback() invalidates the query tree only when
invalItems are added to the plan source. The patch adds the
dependencies in root->glob->invalItems, which standard_planner()
copies into PlannedStmt::invalItems. This is then copied into the
gplan->stmt_list. Thus PlanCacheFuncCallback never invalidates the
query tree. I have verified this under the debugger. Am I missing
something?

I think you are right. Maybe my explanation was not enough, sorry for
that. I just wanted to mention that Amit's patch would invalidate the
generic plan as well as the rewritten query tree.

I looked at your patch closely. You added code to track dependencies on
FDW-related objects to createplan.c, but I think it would be more
appropriate to put that code in setrefs.c like the attached. I think
record_foreignscan_plan_dependencies in your patch would be a bit
inefficient because that tracks such dependencies redundantly in the
case where the given ForeignScan has an outer subplan, so I optimized
that in the attached. (Also some regression tests for that were added.)

I think it would be a bit inefficient to use PlanCacheFuncCallback as
the inval callback function for those caches, because that checks the
inval-item list for the rewritten query tree, but any updates on such
catalogs wouldn't affect the query tree. So, I added a new callback
function for those caches that is much like PlanCacheFuncCallback but
skips checking the list for the query tree. I updated some comments also.

Best regards,
Etsuro Fujita

I looked at your patch closely. You added code to track dependencies on
FDW-related objects to createplan.c, but I think it would be more
appropriate to put that code in setrefs.c like the attached. I think
record_foreignscan_plan_dependencies in your patch would be a bit
inefficient because that tracks such dependencies redundantly in the case
where the given ForeignScan has an outer subplan, so I optimized that in the
attached. (Also some regression tests for that were added.)

Thanks for fixing the inefficiency.

+   /*
+    * Record dependencies on FDW-related objects.  If an outer subplan
+    * exists, that would be done in the processing of its baserels, so skip
+    * that.
+    */
I think, we need a bit more explanation here. How about rewording it
as "Record dependencies on FDW-related objects. If an outer subplan
exists, the dependencies would be recorded while processing the
foreign plans for the base foreign relations in the subplan. Hence
skip that here."

+ * Currently, we track exactly the dependencies of plans on relations,
+ * user-defined functions and FDW-related objects.  On relcache invalidation
+ * events or pg_proc syscache invalidation events, we invalidate just those
+ * plans that depend on the particular object being modified.  (Note: this
+ * scheme assumes that any table modification that requires replanning will
+ * generate a relcache inval event.)  We also watch for inval events on
+ * certain other system catalogs, such as pg_namespace; but for them, our
+ * response is just to invalidate all plans.  We expect updates on those
+ * catalogs to be infrequent enough that more-detailed tracking is not worth
+ * the effort.

Just like you have added FDW-related objects in the first sentence,
reference to those needs to be added in the second sentence as well.
Or you want to start the second sentence as "When the relevant caches
are invalidated, we invalidate ..." so that those two sentences will
remain in sync even after additions/deletions to the first sentence.

I think it would be a bit inefficient to use PlanCacheFuncCallback as the
inval callback function for those caches, because that checks the inval-item
list for the rewritten query tree, but any updates on such catalogs wouldn't
affect the query tree.

I am not sure about that. Right now it seems that only the plans are
affected, but can we say that for all FDWs?

So, I added a new callback function for those caches
that is much like PlanCacheFuncCallback but skips checking the list for the
query tree. I updated some comments also.

I am not sure that the inefficiency because of an extra loop on
plansource->invalItems is a good reason to duplicate most of the code
in PlanCacheFuncCallback(). IMO, maintaining that extra function and
the risk of bugs because of not keeping those two functions in sync
outweigh the small not-so-frequent gain. The name of function may be
changed to be more generic, instead of current one referring Func.

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

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