On 2016/10/18 22:04, Ashutosh Bapat wrote:
On Tue, Oct 18, 2016 at 6:18 PM, Etsuro Fujita
<fujita.ets...@lab.ntt.co.jp> wrote:
On 2016/10/17 20:12, Ashutosh Bapat wrote:
On 2016/10/07 10:26, Amit Langote wrote:
I think this (v4) patch is in the best shape so far.


+1, except one small change.

The comment "... On relcache invalidation events or the relevant
syscache invalidation events, ..." specifies relcache separately. I
think, it should either needs to specify all the syscaches or just
mention "On corresponding syscache invalidation events, ...".

Attached patch uses the later version. Let me know if this looks good.
If you are fine, I think we should mark this as "Ready for committer".

I'm not sure that is a good idea because ISTM the comments there use the
words "syscache" and "relcache" properly.  I'd like to leave that for
committers.

Fine with me.

OK

One thing I'd like to propose to improve the patch is to update the
following comments for PlanCacheFuncCallback: "Note that the coding would
support use for multiple caches, but right now only user-defined functions
are tracked this way.".  We now use this for tracking FDW-related objects as
well, so the comments needs to be updated. Please find attached an updated
version.

Fine with me.

OK

Sorry for speaking this now, but one thing I'm not sure about the patch is
whether we should track the dependencies on FDW-related objects more
accurately; we modified extract_query_dependencies so that the query tree's
dependencies are tracked, regardless of the command type, but the query tree
would be only affected by those objects in AddForeignUpdateTargets, so it
would be enough to collect the dependencies for the case where the given
query is UPDATE/DELETE.  But I thought the patch would be probably fine as
proposed, because we expect updates on such catalogs to be infrequent.  (I
guess the changes needed for the accuracy would be small, though.)

In fact, I am not in
favour of tracking the query dependencies for UPDATE/DELETE since we
don't have any concrete example as to when that would be needed.

Right, but as I said before, some FDW might consult FDW options stored in those objects during AddForeignUpdateTargets, so we should do that.

Besides
that, I modified add_rte_to_flat_rtable so that the plan's dependencies are
tracked, but that would lead to tracking the dependencies of unreferenced
foreign tables in dead subqueries or the dependencies of foreign tables
excluded from the plan by eg, constraint exclusion.  But I thought that
would be also OK by the same reason as above.  (Another reason for that was
it seemed better to me to collect the dependencies in the same place as for
relation OIDs.)

If those unreferenced relations become references because of the
changes in options, we will require those query dependencies to be
recorded. So, if we are recording query dependencies, we should record
the ones on unreferenced relations as well.

I mean plan dependencies here, not query dependencies.

Having said that, I like the latest version (v6), so I'd vote for marking this as Ready For Committer.

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

Reply via email to