Deleting no-op CoerceToDomains from plan trees

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Deleting no-op CoerceToDomains from plan trees
Date: 2018-12-06 18:48:44
Message-ID: 19958.1544122124@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Attached is a complete patch for one of the tasks discussed in [1],
namely dropping CoerceToDomain nodes from plan trees if their domains
have no constraints that need to be checked.

After I fleshed out the original patch to include sinval signaling for
changes of domain constraints, I was surprised to find that the existing
test case in domain.sql still failed. The reason turned out to be that
the CoerceToDomain node was not being done as part of a normal plpgsql
expression, but within a cast expression cached by plpgsql's
get_cast_hashentry() function ... and that code has no provision for
cache flushes.

I concluded that the best way to fix that was to create new functionality
in plancache.c for caching expression trees with appropriate invalidation
support, so this patch includes that. (Conceivably that should be a
separate patch, but it didn't really seem worth the trouble.)

As patched, get_cast_hashentry() is the only user of the new plancache
code. I looked around at other callers of expression_planner() to see
if anything else was caching the result for more than one query.
I found such a caller in typcache.c's domain constraint caching, but
as discussed in [2], it's not really clear that there's any point in
changing that code; I just added a comment about the issue instead.

(I also noted some callers in relation partitioning code, which
I'm a tad suspicious of, mainly because it's not apparent to me why
that code should be executing arbitrary expressions in the first
place. But if there's anything broken there, it seems like material
for a separate discussion.)

Some other notes for review:

* In typecmds.c, I fixed some inconsistency about whether the various
subcommands of ALTER DOMAIN released catalog locks or not.

* I used a dlist to thread CachedExpressions together in plancache.c,
and failed to resist the temptation to change CachedPlanSources to
use a dlist as well. That eliminates a potential performance problem
in DropCachedPlan, though I'm not sure how important that is.

regards, tom lane

[1] https://www.postgresql.org/message-id/5978.1544030694@sss.pgh.pa.us
[2] https://www.postgresql.org/message-id/12539.1544107316@sss.pgh.pa.us

Attachment Content-Type Size
remove-useless-CoerceToDomains-1.patch text/x-diff 41.5 KB

Browse pgsql-hackers by date

  From Date Subject
Next Message Alexander Korotkov 2018-12-06 19:56:23 Re: Connections hang indefinitely while taking a gin index's LWLock buffer_content lock
Previous Message Jeremy Finzel 2018-12-06 18:36:52 How do I get altered object from GRANT event trigger?