Thread: plpgsql versus domains
At the behest of Salesforce, I've been looking into improving plpgsql's
handling of variables of domain types, which is currently pretty awful.
It's really slow, because any assignment to a domain variable goes through
the slow double-I/O-conversion path in exec_cast_value, even if no actual
work is needed. And I noticed that the domain's constraints get looked up
only once per function per session; for example this script gets
unexpected results:
create domain di as int;
create function foo1(int) returns di as $$
declare d di;
begin d := $1; return d;
end
$$ language plpgsql immutable;
select foo1(0); -- call once to set up cache
alter domain di add constraint pos check (value > 0);
select 0::di; -- fails, as expected
select foo1(0); -- should fail, does not
\c -
select foo1(0); -- now it fails
The reason for this is that domain_in looks up the domain's constraints
and caches them under the calling FmgrInfo struct. That behavior was
designed to ensure we'd do just one constraint-lookup per query, which
I think is reasonable. But plpgsql sets up an FmgrInfo in the variable's
PLpgSQL_type record, which persists for the whole session unless the
function's parse tree is flushed for some reason. So the constraints
only get looked up once.
The rough sketch I have in mind for fixing this is:
1. Arrange to cache the constraints for domain types in typcache.c,
so as to reduce the number of trips to the actual catalogs. I think
typcache.c will have to flush these cache items upon seeing any sinval
change in pg_constraint, but that's still cheaper than searching
pg_constraint for every query.
2. In plpgsql, explicitly model a domain type as being its base type
plus some constraints --- that is, stop depending on domain_in() to
enforce the constraints, and do it ourselves. This would mean that
the FmgrInfo in a PLpgSQL_type struct would reference the base type's
input function rather than domain_in, so we'd not have to be afraid
to cache that. The constraints would be represented as a separate list,
which we'd arrange to fetch from the typcache once per transaction.
(I think checking for new constraints once per transaction is sufficient
for reasonable situations, though I suppose that could be argued about.)
The advantage of this approach is first that we could avoid an I/O
conversion when the incoming value to be assigned matches the domain's
base type, which is the typical case; and second that a domain without
any CHECK constraints would become essentially free, which is also a
common case.
3. We could still have domains.c responsible for most of the details;
the domain_check() API may be good enough as-is, though it seems to lack
any simple way to force re-lookup of the domain constraints once per xact.
Thoughts, better ideas?
Given the lack of field complaints, I don't feel a need to try to
back-patch a fix for this, but I do want to see it fixed for 9.5.
regards, tom lane
Hi
2015-02-26 18:31 GMT+01:00 Tom Lane <tgl@sss.pgh.pa.us>:
+1
At the behest of Salesforce, I've been looking into improving plpgsql's
handling of variables of domain types, which is currently pretty awful.
It's really slow, because any assignment to a domain variable goes through
the slow double-I/O-conversion path in exec_cast_value, even if no actual
work is needed. And I noticed that the domain's constraints get looked up
only once per function per session; for example this script gets
unexpected results:
create domain di as int;
create function foo1(int) returns di as $$
declare d di;
begin
d := $1;
return d;
end
$$ language plpgsql immutable;
select foo1(0); -- call once to set up cache
alter domain di add constraint pos check (value > 0);
select 0::di; -- fails, as expected
select foo1(0); -- should fail, does not
\c -
select foo1(0); -- now it fails
The reason for this is that domain_in looks up the domain's constraints
and caches them under the calling FmgrInfo struct. That behavior was
designed to ensure we'd do just one constraint-lookup per query, which
I think is reasonable. But plpgsql sets up an FmgrInfo in the variable's
PLpgSQL_type record, which persists for the whole session unless the
function's parse tree is flushed for some reason. So the constraints
only get looked up once.
The rough sketch I have in mind for fixing this is:
1. Arrange to cache the constraints for domain types in typcache.c,
so as to reduce the number of trips to the actual catalogs. I think
typcache.c will have to flush these cache items upon seeing any sinval
change in pg_constraint, but that's still cheaper than searching
pg_constraint for every query.
2. In plpgsql, explicitly model a domain type as being its base type
plus some constraints --- that is, stop depending on domain_in() to
enforce the constraints, and do it ourselves. This would mean that
the FmgrInfo in a PLpgSQL_type struct would reference the base type's
input function rather than domain_in, so we'd not have to be afraid
to cache that. The constraints would be represented as a separate list,
which we'd arrange to fetch from the typcache once per transaction.
(I think checking for new constraints once per transaction is sufficient
for reasonable situations, though I suppose that could be argued about.)
The advantage of this approach is first that we could avoid an I/O
conversion when the incoming value to be assigned matches the domain's
base type, which is the typical case; and second that a domain without
any CHECK constraints would become essentially free, which is also a
common case.
I like this variant
There can be some good optimization with scalar types: text, varchars, numbers and reduce IO cast.
3. We could still have domains.c responsible for most of the details;
the domain_check() API may be good enough as-is, though it seems to lack
any simple way to force re-lookup of the domain constraints once per xact.
Thoughts, better ideas?
Given the lack of field complaints, I don't feel a need to try to
back-patch a fix for this, but I do want to see it fixed for 9.5.
+1
Regards
Pavel
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
Hi, On 2015-02-26 12:31:46 -0500, Tom Lane wrote: > It's really slow, because any assignment to a domain variable goes through > the slow double-I/O-conversion path in exec_cast_value, even if no actual > work is needed. Hm, that's surely not nice for some types. Doing syscache lookups every time can't help either. > And I noticed that the domain's constraints get looked up > only once per function per session; for example this script gets > unexpected results: > The reason for this is that domain_in looks up the domain's constraints > and caches them under the calling FmgrInfo struct. That's probably far from the only such case we have :( > 1. Arrange to cache the constraints for domain types in typcache.c, > so as to reduce the number of trips to the actual catalogs. I think > typcache.c will have to flush these cache items upon seeing any sinval > change in pg_constraint, but that's still cheaper than searching > pg_constraint for every query. That sounds sane. > 2. In plpgsql, explicitly model a domain type as being its base type > plus some constraints --- that is, stop depending on domain_in() to > enforce the constraints, and do it ourselves. This would mean that > the FmgrInfo in a PLpgSQL_type struct would reference the base type's > input function rather than domain_in, so we'd not have to be afraid > to cache that. The constraints would be represented as a separate list, > which we'd arrange to fetch from the typcache once per transaction. Hm. A bit sad to open code that in plpgsql instead of making it available more generally. > 3. We could still have domains.c responsible for most of the details; > the domain_check() API may be good enough as-is, though it seems to lack > any simple way to force re-lookup of the domain constraints once per xact. > > Thoughts, better ideas? You're probably going to kill me because of the increased complexity, but how about making typecache.c smarter, more in the vein of relcache.c, and store the relevant info in there? And then, to avoid repeated lookups, store a reference to that in fcinfo? The lifetime of objects wouldn't be entirely trivial, but it doesn't sound impossible. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Andres Freund <andres@2ndquadrant.com> writes:
> On 2015-02-26 12:31:46 -0500, Tom Lane wrote:
>> 2. In plpgsql, explicitly model a domain type as being its base type
>> plus some constraints --- that is, stop depending on domain_in() to
>> enforce the constraints, and do it ourselves. This would mean that
>> the FmgrInfo in a PLpgSQL_type struct would reference the base type's
>> input function rather than domain_in, so we'd not have to be afraid
>> to cache that. The constraints would be represented as a separate list,
>> which we'd arrange to fetch from the typcache once per transaction.
> Hm. A bit sad to open code that in plpgsql instead of making it
> available more generally.
The actual checking wouldn't be open-coded, it would come from
domain_check() (or possibly a modified version of that). It is true
that plpgsql would know more about domains than it does today, but
I'm not sure I see another use-case for this type of logic.
To some extent this is a workaround for the fact that plpgsql does type
conversions the way it does (ie, by I/O rather than by using the parser's
cast mechanisms). We've talked about changing that, but people seem to
be afraid of the compatibility consequences, and I'm not planning to fight
two compatibility battles concurrently ;-)
> You're probably going to kill me because of the increased complexity,
> but how about making typecache.c smarter, more in the vein of
> relcache.c, and store the relevant info in there?
I thought that's what I was proposing in point #1.
regards, tom lane
On 2015-02-26 13:53:18 -0500, Tom Lane wrote: > Andres Freund <andres@2ndquadrant.com> writes: > > Hm. A bit sad to open code that in plpgsql instead of making it > > available more generally. > > The actual checking wouldn't be open-coded, it would come from > domain_check() (or possibly a modified version of that). It is true > that plpgsql would know more about domains than it does today, but It'd still teach plpgsql more about types than it knows right now. But on a second thought that ship has sailed long ago - and the amount of knowledge seems relatively small. There's much more stuff about composites there already. > and I'm not planning to fight two compatibility battles concurrently ;-) Heh ;) > > You're probably going to kill me because of the increased complexity, > > but how about making typecache.c smarter, more in the vein of > > relcache.c, and store the relevant info in there? > > I thought that's what I was proposing in point #1. Sure, but my second point was to avoid duplicating that information into ->fcinfo and such and instead reference the typecache entry from there. So that if the typecache entry is being rebuilt (a new mechanism I'm afraid), whatever uses ->fcinfo gets the updated functions/definition. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Andres Freund <andres@2ndquadrant.com> writes:
> On 2015-02-26 13:53:18 -0500, Tom Lane wrote:
>> I thought that's what I was proposing in point #1.
> Sure, but my second point was to avoid duplicating that information into
> ->fcinfo and such and instead reference the typecache entry from
> there. So that if the typecache entry is being rebuilt (a new mechanism
> I'm afraid), whatever uses ->fcinfo gets the updated
> functions/definition.
The trick there would be that if we don't want to copy then we'd need a
reference counting mechanism, and FmgrInfos aren't used in a way that
would allow that to work easily. (Whatever the typcache is holding onto
clearly must be long-lived, but you do want an obsoleted one to go away
once there are no more FmgrInfos referencing it.)
I was just thinking though that we could possibly solve that if we went
ahead and invented the memory context reset callback mechanism that
I suggested in another thread a week or two back. Then you could imagine
that when a domain-checking function caches a reference to a "domain
constraints info" object in its FmgrInfo, it could increment a refcount
on the info object, and register a callback on the context containing the
FmgrInfo to release that refcount.
A different approach would be to try to use the ResourceOwner mechanism
to track these info objects. But I think that does not work nicely for
plpgsql; at least not unless it creates a ResourceOwner for every
function, which seems kinda messy.
regards, tom lane
2015-02-26 19:53 GMT+01:00 Tom Lane <tgl@sss.pgh.pa.us>:
Andres Freund <andres@2ndquadrant.com> writes:
> On 2015-02-26 12:31:46 -0500, Tom Lane wrote:
>> 2. In plpgsql, explicitly model a domain type as being its base type
>> plus some constraints --- that is, stop depending on domain_in() to
>> enforce the constraints, and do it ourselves. This would mean that
>> the FmgrInfo in a PLpgSQL_type struct would reference the base type's
>> input function rather than domain_in, so we'd not have to be afraid
>> to cache that. The constraints would be represented as a separate list,
>> which we'd arrange to fetch from the typcache once per transaction.
> Hm. A bit sad to open code that in plpgsql instead of making it
> available more generally.
The actual checking wouldn't be open-coded, it would come from
domain_check() (or possibly a modified version of that). It is true
that plpgsql would know more about domains than it does today, but
I'm not sure I see another use-case for this type of logic.
To some extent this is a workaround for the fact that plpgsql does type
conversions the way it does (ie, by I/O rather than by using the parser's
cast mechanisms). We've talked about changing that, but people seem to
be afraid of the compatibility consequences, and I'm not planning to fight
two compatibility battles concurrently ;-)
I understand, but in this case, a compatibility can be enforced simply - we can support "a only know cast" mode and "IO cast" mode.
IO cast is simple for lot of people, but there is a lot of performance issues and few errors related to this topic. But this is offtopic now.
But, what can be related - for plpgsql_check is necessary some simple check of a assigning - that should to return error or warning
This part of plpgsql_check is too complex now.
Regards
Pavel
> You're probably going to kill me because of the increased complexity,
> but how about making typecache.c smarter, more in the vein of
> relcache.c, and store the relevant info in there?
I thought that's what I was proposing in point #1.
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 Thu, Feb 26, 2015 at 1:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > To some extent this is a workaround for the fact that plpgsql does type > conversions the way it does (ie, by I/O rather than by using the parser's > cast mechanisms). We've talked about changing that, but people seem to > be afraid of the compatibility consequences, and I'm not planning to fight > two compatibility battles concurrently ;-) A sensible plan, but since we're here: - I do agree that changing the way PL/pgsql does those type conversions is scary. I can't predict what will break, and there's no getting around the scariness of that. - On the other hand, I do think it would be good to change it, because this sucks: rhaas=# do $$ declare x int; begin x := (3.0::numeric)/(1.0::numeric); end $$; ERROR: invalid input syntax for integer: "3.0000000000000000" CONTEXT: PL/pgSQL function inline_code_block line 1 at assignment I didn't realize that this issue had even been discussed before... -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Feb 26, 2015 at 1:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> To some extent this is a workaround for the fact that plpgsql does type
>> conversions the way it does (ie, by I/O rather than by using the parser's
>> cast mechanisms). We've talked about changing that, but people seem to
>> be afraid of the compatibility consequences, and I'm not planning to fight
>> two compatibility battles concurrently ;-)
> A sensible plan, but since we're here:
> - I do agree that changing the way PL/pgsql does those type
> conversions is scary. I can't predict what will break, and there's no
> getting around the scariness of that.
> - On the other hand, I do think it would be good to change it, because
> this sucks:
> rhaas=# do $$ declare x int; begin x := (3.0::numeric)/(1.0::numeric); end $$;
> ERROR: invalid input syntax for integer: "3.0000000000000000"
> CONTEXT: PL/pgSQL function inline_code_block line 1 at assignment
If we did want to open that can of worms, my proposal would be to make
plpgsql type conversions work like so:
* If there is a cast pathway known to the core SQL parser, use that mechanism.
* Otherwise, attempt to convert via I/O as we do today.
This seems to minimize the risk of breaking things, although there would
probably be corner cases that work differently (for instance I bet boolean
to text might turn out differently). In the very long run we could perhaps
deprecate and remove the second phase.
regards, tom lane
2015-02-27 21:57 GMT+01:00 Tom Lane <tgl@sss.pgh.pa.us>:
+1
Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Feb 26, 2015 at 1:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> To some extent this is a workaround for the fact that plpgsql does type
>> conversions the way it does (ie, by I/O rather than by using the parser's
>> cast mechanisms). We've talked about changing that, but people seem to
>> be afraid of the compatibility consequences, and I'm not planning to fight
>> two compatibility battles concurrently ;-)
> A sensible plan, but since we're here:
> - I do agree that changing the way PL/pgsql does those type
> conversions is scary. I can't predict what will break, and there's no
> getting around the scariness of that.
> - On the other hand, I do think it would be good to change it, because
> this sucks:
> rhaas=# do $$ declare x int; begin x := (3.0::numeric)/(1.0::numeric); end $$;
> ERROR: invalid input syntax for integer: "3.0000000000000000"
> CONTEXT: PL/pgSQL function inline_code_block line 1 at assignment
If we did want to open that can of worms, my proposal would be to make
plpgsql type conversions work like so:
* If there is a cast pathway known to the core SQL parser, use that
mechanism.
* Otherwise, attempt to convert via I/O as we do today.
This seems to minimize the risk of breaking things, although there would
probably be corner cases that work differently (for instance I bet boolean
to text might turn out differently). In the very long run we could perhaps
deprecate and remove the second phase.
+1
There can be similar solution like plpgsql/sql identifiers priority configuration. Some levels - and what you are proposing should be default.
It works perfectly - and from my view and what I know from my neighborhood, there are no issues.
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
Andres Freund <andres@2ndquadrant.com> writes:
> On 2015-02-26 13:53:18 -0500, Tom Lane wrote:
>> Andres Freund <andres@2ndquadrant.com> writes:
>>> You're probably going to kill me because of the increased complexity,
>>> but how about making typecache.c smarter, more in the vein of
>>> relcache.c, and store the relevant info in there?
>> I thought that's what I was proposing in point #1.
> Sure, but my second point was to avoid duplicating that information into
> ->fcinfo and such and instead reference the typecache entry from
> there. So that if the typecache entry is being rebuilt (a new mechanism
> I'm afraid), whatever uses ->fcinfo gets the updated
> functions/definition.
Here's a draft patch for that. This fixes the delayed-domain-constraint
problem pretty thoroughly, in that any attempt to check a domain's
constraints will use fully up-to-date info.
This is the first attempt at weaponizing the memory context reset/delete
feature, and I'm fairly happy with it, except for one thing: I had to
#include utils/memnodes.h into typcache.h in order to preserve the
intended property that the callback structs could be included directly
into structs using them. Now, that's not awful in itself, because
typcache.h isn't used everywhere; but if this feature gets popular we'll
find memnodes.h being included pretty much everywhere, which is not so
great. I'm thinking about moving struct MemoryContextCallback and the
extern for MemoryContextRegisterResetCallback into palloc.h to avoid this.
Maybe that's too far in the other direction. Thoughts? Compromise ideas?
Also, instrumenting the code showed that TypeCacheConstrCallback gets
called quite a lot during the standard regression tests, which is why
I went out of my way to make it quick. Almost all of those cache flushes
are from non-domain-related updates on pg_type or pg_constraint, so
they're not really necessary from a logical perspective, and they're
surely going to hurt performance for heavy users of domains. I think to
fix this we'd have to split pg_constraint into two catalogs, one for table
constraints and one for domain constraints; which would be a good thing
anyway from a normal-form-theory perspective. And we'd have to get rid of
pg_type.typnotnull and instead store domain NOT NULL constraints in this
hypothetical domain constraint catalog. I don't plan to do anything about
that myself right now, because I'm not sure that production databases
would have the kind of traffic on pg_type and pg_constraint that the
regression tests exhibit. But at some point we might have to fix it.
regards, tom lane
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 07ab4b4..7455020 100644
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
*************** ATExecAddColumn(List **wqueue, AlteredTa
*** 4824,4830 ****
{
defval = (Expr *) build_column_default(rel, attribute.attnum);
! if (!defval && GetDomainConstraints(typeOid) != NIL)
{
Oid baseTypeId;
int32 baseTypeMod;
--- 4824,4830 ----
{
defval = (Expr *) build_column_default(rel, attribute.attnum);
! if (!defval && DomainHasConstraints(typeOid))
{
Oid baseTypeId;
int32 baseTypeMod;
*************** ATColumnChangeRequiresRewrite(Node *expr
*** 7778,7784 ****
{
CoerceToDomain *d = (CoerceToDomain *) expr;
! if (GetDomainConstraints(d->resulttype) != NIL)
return true;
expr = (Node *) d->arg;
}
--- 7778,7784 ----
{
CoerceToDomain *d = (CoerceToDomain *) expr;
! if (DomainHasConstraints(d->resulttype))
return true;
expr = (Node *) d->arg;
}
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index b77e1b4..d800033 100644
*** a/src/backend/commands/typecmds.c
--- b/src/backend/commands/typecmds.c
***************
*** 59,65 ****
#include "executor/executor.h"
#include "miscadmin.h"
#include "nodes/makefuncs.h"
- #include "optimizer/planner.h"
#include "optimizer/var.h"
#include "parser/parse_coerce.h"
#include "parser/parse_collate.h"
--- 59,64 ----
*************** domainAddConstraint(Oid domainOid, Oid d
*** 3081,3206 ****
return ccbin;
}
- /*
- * GetDomainConstraints - get a list of the current constraints of domain
- *
- * Returns a possibly-empty list of DomainConstraintState nodes.
- *
- * This is called by the executor during plan startup for a CoerceToDomain
- * expression node. The given constraints will be checked for each value
- * passed through the node.
- *
- * We allow this to be called for non-domain types, in which case the result
- * is always NIL.
- */
- List *
- GetDomainConstraints(Oid typeOid)
- {
- List *result = NIL;
- bool notNull = false;
- Relation conRel;
-
- conRel = heap_open(ConstraintRelationId, AccessShareLock);
-
- for (;;)
- {
- HeapTuple tup;
- HeapTuple conTup;
- Form_pg_type typTup;
- ScanKeyData key[1];
- SysScanDesc scan;
-
- tup = SearchSysCache1(TYPEOID, ObjectIdGetDatum(typeOid));
- if (!HeapTupleIsValid(tup))
- elog(ERROR, "cache lookup failed for type %u", typeOid);
- typTup = (Form_pg_type) GETSTRUCT(tup);
-
- if (typTup->typtype != TYPTYPE_DOMAIN)
- {
- /* Not a domain, so done */
- ReleaseSysCache(tup);
- break;
- }
-
- /* Test for NOT NULL Constraint */
- if (typTup->typnotnull)
- notNull = true;
-
- /* Look for CHECK Constraints on this domain */
- ScanKeyInit(&key[0],
- Anum_pg_constraint_contypid,
- BTEqualStrategyNumber, F_OIDEQ,
- ObjectIdGetDatum(typeOid));
-
- scan = systable_beginscan(conRel, ConstraintTypidIndexId, true,
- NULL, 1, key);
-
- while (HeapTupleIsValid(conTup = systable_getnext(scan)))
- {
- Form_pg_constraint c = (Form_pg_constraint) GETSTRUCT(conTup);
- Datum val;
- bool isNull;
- Expr *check_expr;
- DomainConstraintState *r;
-
- /* Ignore non-CHECK constraints (presently, shouldn't be any) */
- if (c->contype != CONSTRAINT_CHECK)
- continue;
-
- /*
- * Not expecting conbin to be NULL, but we'll test for it anyway
- */
- val = fastgetattr(conTup, Anum_pg_constraint_conbin,
- conRel->rd_att, &isNull);
- if (isNull)
- elog(ERROR, "domain \"%s\" constraint \"%s\" has NULL conbin",
- NameStr(typTup->typname), NameStr(c->conname));
-
- check_expr = (Expr *) stringToNode(TextDatumGetCString(val));
-
- /* ExecInitExpr assumes we've planned the expression */
- check_expr = expression_planner(check_expr);
-
- r = makeNode(DomainConstraintState);
- r->constrainttype = DOM_CONSTRAINT_CHECK;
- r->name = pstrdup(NameStr(c->conname));
- r->check_expr = ExecInitExpr(check_expr, NULL);
-
- /*
- * use lcons() here because constraints of lower domains should be
- * applied earlier.
- */
- result = lcons(r, result);
- }
-
- systable_endscan(scan);
-
- /* loop to next domain in stack */
- typeOid = typTup->typbasetype;
- ReleaseSysCache(tup);
- }
-
- heap_close(conRel, AccessShareLock);
-
- /*
- * Only need to add one NOT NULL check regardless of how many domains in
- * the stack request it.
- */
- if (notNull)
- {
- DomainConstraintState *r = makeNode(DomainConstraintState);
-
- r->constrainttype = DOM_CONSTRAINT_NOTNULL;
- r->name = pstrdup("NOT NULL");
- r->check_expr = NULL;
-
- /* lcons to apply the nullness check FIRST */
- result = lcons(r, result);
- }
-
- return result;
- }
-
/*
* Execute ALTER TYPE RENAME
--- 3080,3085 ----
diff --git a/src/backend/executor/execQual.c b/src/backend/executor/execQual.c
index fec76d4..d94fe58 100644
*** a/src/backend/executor/execQual.c
--- b/src/backend/executor/execQual.c
***************
*** 41,47 ****
#include "access/tupconvert.h"
#include "catalog/objectaccess.h"
#include "catalog/pg_type.h"
- #include "commands/typecmds.h"
#include "executor/execdebug.h"
#include "executor/nodeSubplan.h"
#include "funcapi.h"
--- 41,46 ----
*************** ExecEvalCoerceToDomain(CoerceToDomainSta
*** 3929,3935 ****
if (isDone && *isDone == ExprEndResult)
return result; /* nothing to check */
! foreach(l, cstate->constraints)
{
DomainConstraintState *con = (DomainConstraintState *) lfirst(l);
--- 3928,3937 ----
if (isDone && *isDone == ExprEndResult)
return result; /* nothing to check */
! /* Make sure we have up-to-date constraints */
! UpdateDomainConstraintRef(cstate->constraint_ref);
!
! foreach(l, cstate->constraint_ref->constraints)
{
DomainConstraintState *con = (DomainConstraintState *) lfirst(l);
*************** ExecInitExpr(Expr *node, PlanState *pare
*** 5050,5056 ****
cstate->xprstate.evalfunc = (ExprStateEvalFunc) ExecEvalCoerceToDomain;
cstate->arg = ExecInitExpr(ctest->arg, parent);
! cstate->constraints = GetDomainConstraints(ctest->resulttype);
state = (ExprState *) cstate;
}
break;
--- 5052,5063 ----
cstate->xprstate.evalfunc = (ExprStateEvalFunc) ExecEvalCoerceToDomain;
cstate->arg = ExecInitExpr(ctest->arg, parent);
! /* We spend an extra palloc to reduce header inclusions */
! cstate->constraint_ref = (DomainConstraintRef *)
! palloc(sizeof(DomainConstraintRef));
! InitDomainConstraintRef(ctest->resulttype,
! cstate->constraint_ref,
! CurrentMemoryContext);
state = (ExprState *) cstate;
}
break;
diff --git a/src/backend/utils/adt/domains.c b/src/backend/utils/adt/domains.c
index d84d4e8..9c8eb11 100644
*** a/src/backend/utils/adt/domains.c
--- b/src/backend/utils/adt/domains.c
***************
*** 12,21 ****
* The overhead required for constraint checking can be high, since examining
* the catalogs to discover the constraints for a given domain is not cheap.
* We have three mechanisms for minimizing this cost:
! * 1. In a nest of domains, we flatten the checking of all the levels
! * into just one operation.
! * 2. We cache the list of constraint items in the FmgrInfo struct
! * passed by the caller.
* 3. If there are CHECK constraints, we cache a standalone ExprContext
* to evaluate them in.
*
--- 12,20 ----
* The overhead required for constraint checking can be high, since examining
* the catalogs to discover the constraints for a given domain is not cheap.
* We have three mechanisms for minimizing this cost:
! * 1. We rely on the typcache to keep up-to-date copies of the constraints.
! * 2. In a nest of domains, we flatten the checking of all the levels
! * into just one operation (the typcache does this for us).
* 3. If there are CHECK constraints, we cache a standalone ExprContext
* to evaluate them in.
*
***************
*** 33,44 ****
#include "access/htup_details.h"
#include "catalog/pg_type.h"
- #include "commands/typecmds.h"
#include "executor/executor.h"
#include "lib/stringinfo.h"
#include "utils/builtins.h"
#include "utils/lsyscache.h"
#include "utils/syscache.h"
/*
--- 32,43 ----
#include "access/htup_details.h"
#include "catalog/pg_type.h"
#include "executor/executor.h"
#include "lib/stringinfo.h"
#include "utils/builtins.h"
#include "utils/lsyscache.h"
#include "utils/syscache.h"
+ #include "utils/typcache.h"
/*
*************** typedef struct DomainIOData
*** 52,59 ****
Oid typioparam;
int32 typtypmod;
FmgrInfo proc;
! /* List of constraint items to check */
! List *constraint_list;
/* Context for evaluating CHECK constraints in */
ExprContext *econtext;
/* Memory context this cache is in */
--- 51,58 ----
Oid typioparam;
int32 typtypmod;
FmgrInfo proc;
! /* Reference to cached list of constraint items to check */
! DomainConstraintRef constraint_ref;
/* Context for evaluating CHECK constraints in */
ExprContext *econtext;
/* Memory context this cache is in */
*************** domain_state_setup(DomainIOData *my_extr
*** 69,75 ****
MemoryContext mcxt)
{
Oid baseType;
- MemoryContext oldcontext;
/* Mark cache invalid */
my_extra->domain_type = InvalidOid;
--- 68,73 ----
*************** domain_state_setup(DomainIOData *my_extr
*** 95,103 ****
fmgr_info_cxt(my_extra->typiofunc, &my_extra->proc, mcxt);
/* Look up constraints for domain */
! oldcontext = MemoryContextSwitchTo(mcxt);
! my_extra->constraint_list = GetDomainConstraints(domainType);
! MemoryContextSwitchTo(oldcontext);
/* We don't make an ExprContext until needed */
my_extra->econtext = NULL;
--- 93,99 ----
fmgr_info_cxt(my_extra->typiofunc, &my_extra->proc, mcxt);
/* Look up constraints for domain */
! InitDomainConstraintRef(domainType, &my_extra->constraint_ref, mcxt);
/* We don't make an ExprContext until needed */
my_extra->econtext = NULL;
*************** domain_check_input(Datum value, bool isn
*** 118,124 ****
ExprContext *econtext = my_extra->econtext;
ListCell *l;
! foreach(l, my_extra->constraint_list)
{
DomainConstraintState *con = (DomainConstraintState *) lfirst(l);
--- 114,123 ----
ExprContext *econtext = my_extra->econtext;
ListCell *l;
! /* Make sure we have up-to-date constraints */
! UpdateDomainConstraintRef(&my_extra->constraint_ref);
!
! foreach(l, my_extra->constraint_ref.constraints)
{
DomainConstraintState *con = (DomainConstraintState *) lfirst(l);
diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c
index 0492718..44b5937 100644
*** a/src/backend/utils/cache/typcache.c
--- b/src/backend/utils/cache/typcache.c
***************
*** 18,32 ****
*
* Once created, a type cache entry lives as long as the backend does, so
* there is no need for a call to release a cache entry. If the type is
! * dropped, the cache entry simply becomes wasted storage. (For present uses,
! * it would be okay to flush type cache entries at the ends of transactions,
! * if we needed to reclaim space.)
*
* We have some provisions for updating cache entries if the stored data
* becomes obsolete. Information dependent on opclasses is cleared if we
* detect updates to pg_opclass. We also support clearing the tuple
* descriptor and operator/function parts of a rowtype's cache entry,
* since those may need to change as a consequence of ALTER TABLE.
*
*
* Portions Copyright (c) 1996-2015, PostgreSQL Global Development Group
--- 18,33 ----
*
* Once created, a type cache entry lives as long as the backend does, so
* there is no need for a call to release a cache entry. If the type is
! * dropped, the cache entry simply becomes wasted storage. This is not
! * expected to happen often, and assuming that typcache entries are good
! * permanently allows caching pointers to them in long-lived places.
*
* We have some provisions for updating cache entries if the stored data
* becomes obsolete. Information dependent on opclasses is cleared if we
* detect updates to pg_opclass. We also support clearing the tuple
* descriptor and operator/function parts of a rowtype's cache entry,
* since those may need to change as a consequence of ALTER TABLE.
+ * Domain constraint changes are also tracked properly.
*
*
* Portions Copyright (c) 1996-2015, PostgreSQL Global Development Group
***************
*** 46,61 ****
--- 47,66 ----
#include "access/htup_details.h"
#include "access/nbtree.h"
#include "catalog/indexing.h"
+ #include "catalog/pg_constraint.h"
#include "catalog/pg_enum.h"
#include "catalog/pg_operator.h"
#include "catalog/pg_range.h"
#include "catalog/pg_type.h"
#include "commands/defrem.h"
+ #include "executor/executor.h"
+ #include "optimizer/planner.h"
#include "utils/builtins.h"
#include "utils/catcache.h"
#include "utils/fmgroids.h"
#include "utils/inval.h"
#include "utils/lsyscache.h"
+ #include "utils/memutils.h"
#include "utils/rel.h"
#include "utils/snapmgr.h"
#include "utils/syscache.h"
***************
*** 65,70 ****
--- 70,78 ----
/* The main type cache hashtable searched by lookup_type_cache */
static HTAB *TypeCacheHash = NULL;
+ /* List of type cache entries for domain types */
+ static TypeCacheEntry *firstDomainTypeEntry = NULL;
+
/* Private flag bits in the TypeCacheEntry.flags field */
#define TCFLAGS_CHECKED_BTREE_OPCLASS 0x0001
#define TCFLAGS_CHECKED_HASH_OPCLASS 0x0002
*************** static HTAB *TypeCacheHash = NULL;
*** 80,85 ****
--- 88,106 ----
#define TCFLAGS_CHECKED_FIELD_PROPERTIES 0x0800
#define TCFLAGS_HAVE_FIELD_EQUALITY 0x1000
#define TCFLAGS_HAVE_FIELD_COMPARE 0x2000
+ #define TCFLAGS_CHECKED_DOMAIN_CONSTRAINTS 0x4000
+
+ /*
+ * Data stored about a domain type's constraints. Note that we do not create
+ * this struct for the common case of a constraint-less domain; we just set
+ * domainData to NULL to indicate that.
+ */
+ struct DomainConstraintCache
+ {
+ List *constraints; /* list of DomainConstraintState nodes */
+ MemoryContext dccContext; /* memory context holding all associated data */
+ long dccRefCount; /* number of references to this struct */
+ };
/* Private information to support comparisons of enum values */
typedef struct
*************** static int32 NextRecordTypmod = 0; /* n
*** 127,132 ****
--- 148,156 ----
static void load_typcache_tupdesc(TypeCacheEntry *typentry);
static void load_rangetype_info(TypeCacheEntry *typentry);
+ static void load_domaintype_info(TypeCacheEntry *typentry);
+ static void decr_dcc_refcount(DomainConstraintCache *dcc);
+ static void dccref_deletion_callback(void *arg);
static bool array_element_has_equality(TypeCacheEntry *typentry);
static bool array_element_has_compare(TypeCacheEntry *typentry);
static bool array_element_has_hashing(TypeCacheEntry *typentry);
*************** static bool record_fields_have_compare(T
*** 136,141 ****
--- 160,166 ----
static void cache_record_field_properties(TypeCacheEntry *typentry);
static void TypeCacheRelCallback(Datum arg, Oid relid);
static void TypeCacheOpcCallback(Datum arg, int cacheid, uint32 hashvalue);
+ static void TypeCacheConstrCallback(Datum arg, int cacheid, uint32 hashvalue);
static void load_enum_cache_data(TypeCacheEntry *tcache);
static EnumItem *find_enumitem(TypeCacheEnumData *enumdata, Oid arg);
static int enum_oid_cmp(const void *left, const void *right);
*************** lookup_type_cache(Oid type_id, int flags
*** 172,177 ****
--- 197,204 ----
/* Also set up callbacks for SI invalidations */
CacheRegisterRelcacheCallback(TypeCacheRelCallback, (Datum) 0);
CacheRegisterSyscacheCallback(CLAOID, TypeCacheOpcCallback, (Datum) 0);
+ CacheRegisterSyscacheCallback(CONSTROID, TypeCacheConstrCallback, (Datum) 0);
+ CacheRegisterSyscacheCallback(TYPEOID, TypeCacheConstrCallback, (Datum) 0);
/* Also make sure CacheMemoryContext exists */
if (!CacheMemoryContext)
*************** lookup_type_cache(Oid type_id, int flags
*** 217,222 ****
--- 244,256 ----
typentry->typtype = typtup->typtype;
typentry->typrelid = typtup->typrelid;
+ /* If it's a domain, immediately thread it into the domain cache list */
+ if (typentry->typtype == TYPTYPE_DOMAIN)
+ {
+ typentry->nextDomain = firstDomainTypeEntry;
+ firstDomainTypeEntry = typentry;
+ }
+
ReleaseSysCache(tp);
}
*************** lookup_type_cache(Oid type_id, int flags
*** 503,508 ****
--- 537,552 ----
load_rangetype_info(typentry);
}
+ /*
+ * If requested, get information about a domain type
+ */
+ if ((flags & TYPECACHE_DOMAIN_INFO) &&
+ (typentry->flags & TCFLAGS_CHECKED_DOMAIN_CONSTRAINTS) == 0 &&
+ typentry->typtype == TYPTYPE_DOMAIN)
+ {
+ load_domaintype_info(typentry);
+ }
+
return typentry;
}
*************** load_rangetype_info(TypeCacheEntry *type
*** 592,597 ****
--- 636,962 ----
/*
+ * load_domaintype_info --- helper routine to set up domain constraint info
+ *
+ * Note: we assume we're called in a relatively short-lived context, so it's
+ * okay to leak data into the current context while scanning pg_constraint.
+ * We build the new DomainConstraintCache data in a context underneath
+ * CurrentMemoryContext, and reparent it under CacheMemoryContext when
+ * complete.
+ */
+ static void
+ load_domaintype_info(TypeCacheEntry *typentry)
+ {
+ Oid typeOid = typentry->type_id;
+ DomainConstraintCache *dcc;
+ bool notNull = false;
+ Relation conRel;
+ MemoryContext oldcxt;
+
+ /*
+ * If we're here, any existing constraint info is stale, so release it.
+ * For safety, be sure to null the link before trying to delete the data.
+ */
+ if (typentry->domainData)
+ {
+ dcc = typentry->domainData;
+ typentry->domainData = NULL;
+ decr_dcc_refcount(dcc);
+ }
+
+ /*
+ * We try to optimize the common case of no domain constraints, so don't
+ * create the dcc object and context until we find a constraint.
+ */
+ dcc = NULL;
+
+ /*
+ * Scan pg_constraint for relevant constraints. We want to find
+ * constraints for not just this domain, but any ancestor domains, so the
+ * outer loop crawls up the domain stack.
+ */
+ conRel = heap_open(ConstraintRelationId, AccessShareLock);
+
+ for (;;)
+ {
+ HeapTuple tup;
+ HeapTuple conTup;
+ Form_pg_type typTup;
+ ScanKeyData key[1];
+ SysScanDesc scan;
+
+ tup = SearchSysCache1(TYPEOID, ObjectIdGetDatum(typeOid));
+ if (!HeapTupleIsValid(tup))
+ elog(ERROR, "cache lookup failed for type %u", typeOid);
+ typTup = (Form_pg_type) GETSTRUCT(tup);
+
+ if (typTup->typtype != TYPTYPE_DOMAIN)
+ {
+ /* Not a domain, so done */
+ ReleaseSysCache(tup);
+ break;
+ }
+
+ /* Test for NOT NULL Constraint */
+ if (typTup->typnotnull)
+ notNull = true;
+
+ /* Look for CHECK Constraints on this domain */
+ ScanKeyInit(&key[0],
+ Anum_pg_constraint_contypid,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(typeOid));
+
+ scan = systable_beginscan(conRel, ConstraintTypidIndexId, true,
+ NULL, 1, key);
+
+ while (HeapTupleIsValid(conTup = systable_getnext(scan)))
+ {
+ Form_pg_constraint c = (Form_pg_constraint) GETSTRUCT(conTup);
+ Datum val;
+ bool isNull;
+ char *constring;
+ Expr *check_expr;
+ DomainConstraintState *r;
+
+ /* Ignore non-CHECK constraints (presently, shouldn't be any) */
+ if (c->contype != CONSTRAINT_CHECK)
+ continue;
+
+ /* Not expecting conbin to be NULL, but we'll test for it anyway */
+ val = fastgetattr(conTup, Anum_pg_constraint_conbin,
+ conRel->rd_att, &isNull);
+ if (isNull)
+ elog(ERROR, "domain \"%s\" constraint \"%s\" has NULL conbin",
+ NameStr(typTup->typname), NameStr(c->conname));
+
+ /* Convert conbin to C string in caller context */
+ constring = TextDatumGetCString(val);
+
+ /* Create the DomainConstraintCache object and context if needed */
+ if (dcc == NULL)
+ {
+ MemoryContext cxt;
+
+ cxt = AllocSetContextCreate(CurrentMemoryContext,
+ "Domain constraints",
+ ALLOCSET_SMALL_INITSIZE,
+ ALLOCSET_SMALL_MINSIZE,
+ ALLOCSET_SMALL_MAXSIZE);
+ dcc = (DomainConstraintCache *)
+ MemoryContextAlloc(cxt, sizeof(DomainConstraintCache));
+ dcc->constraints = NIL;
+ dcc->dccContext = cxt;
+ dcc->dccRefCount = 0;
+ }
+
+ /* Create node trees in DomainConstraintCache's context */
+ oldcxt = MemoryContextSwitchTo(dcc->dccContext);
+
+ check_expr = (Expr *) stringToNode(constring);
+
+ /* ExecInitExpr assumes we've planned the expression */
+ check_expr = expression_planner(check_expr);
+
+ r = makeNode(DomainConstraintState);
+ r->constrainttype = DOM_CONSTRAINT_CHECK;
+ r->name = pstrdup(NameStr(c->conname));
+ r->check_expr = ExecInitExpr(check_expr, NULL);
+
+ /*
+ * Use lcons() here because constraints of parent domains should
+ * be applied earlier.
+ */
+ dcc->constraints = lcons(r, dcc->constraints);
+
+ MemoryContextSwitchTo(oldcxt);
+ }
+
+ systable_endscan(scan);
+
+ /* loop to next domain in stack */
+ typeOid = typTup->typbasetype;
+ ReleaseSysCache(tup);
+ }
+
+ heap_close(conRel, AccessShareLock);
+
+ /*
+ * Only need to add one NOT NULL check regardless of how many domains in
+ * the stack request it.
+ */
+ if (notNull)
+ {
+ DomainConstraintState *r;
+
+ /* Create the DomainConstraintCache object and context if needed */
+ if (dcc == NULL)
+ {
+ MemoryContext cxt;
+
+ cxt = AllocSetContextCreate(CurrentMemoryContext,
+ "Domain constraints",
+ ALLOCSET_SMALL_INITSIZE,
+ ALLOCSET_SMALL_MINSIZE,
+ ALLOCSET_SMALL_MAXSIZE);
+ dcc = (DomainConstraintCache *)
+ MemoryContextAlloc(cxt, sizeof(DomainConstraintCache));
+ dcc->constraints = NIL;
+ dcc->dccContext = cxt;
+ dcc->dccRefCount = 0;
+ }
+
+ /* Create node trees in DomainConstraintCache's context */
+ oldcxt = MemoryContextSwitchTo(dcc->dccContext);
+
+ r = makeNode(DomainConstraintState);
+
+ r->constrainttype = DOM_CONSTRAINT_NOTNULL;
+ r->name = pstrdup("NOT NULL");
+ r->check_expr = NULL;
+
+ /* lcons to apply the nullness check FIRST */
+ dcc->constraints = lcons(r, dcc->constraints);
+
+ MemoryContextSwitchTo(oldcxt);
+ }
+
+ /*
+ * If we made a constraint object, move it into CacheMemoryContext and
+ * attach it to the typcache entry.
+ */
+ if (dcc)
+ {
+ MemoryContextSetParent(dcc->dccContext, CacheMemoryContext);
+ typentry->domainData = dcc;
+ dcc->dccRefCount++; /* count the typcache's reference */
+ }
+
+ /* Either way, the typcache entry's domain data is now valid. */
+ typentry->flags |= TCFLAGS_CHECKED_DOMAIN_CONSTRAINTS;
+ }
+
+ /*
+ * decr_dcc_refcount --- decrement a DomainConstraintCache's refcount,
+ * and free it if no references remain
+ */
+ static void
+ decr_dcc_refcount(DomainConstraintCache *dcc)
+ {
+ Assert(dcc->dccRefCount > 0);
+ if (--(dcc->dccRefCount) <= 0)
+ MemoryContextDelete(dcc->dccContext);
+ }
+
+ /*
+ * Context reset/delete callback for a DomainConstraintRef
+ */
+ static void
+ dccref_deletion_callback(void *arg)
+ {
+ DomainConstraintRef *ref = (DomainConstraintRef *) arg;
+ DomainConstraintCache *dcc = ref->dcc;
+
+ /* Paranoia --- be sure link is nulled before trying to release */
+ if (dcc)
+ {
+ ref->constraints = NIL;
+ ref->dcc = NULL;
+ decr_dcc_refcount(dcc);
+ }
+ }
+
+ /*
+ * InitDomainConstraintRef --- initialize a DomainConstraintRef struct
+ *
+ * Caller must tell us the MemoryContext in which the DomainConstraintRef
+ * lives. The ref will be cleaned up when that context is reset/deleted.
+ */
+ void
+ InitDomainConstraintRef(Oid type_id, DomainConstraintRef *ref,
+ MemoryContext refctx)
+ {
+ /* Look up the typcache entry --- we assume it survives indefinitely */
+ ref->tcache = lookup_type_cache(type_id, TYPECACHE_DOMAIN_INFO);
+ /* For safety, establish the callback before acquiring a refcount */
+ ref->dcc = NULL;
+ ref->callback.func = dccref_deletion_callback;
+ ref->callback.arg = (void *) ref;
+ MemoryContextRegisterResetCallback(refctx, &ref->callback);
+ /* Acquire refcount if there are constraints, and set up exported list */
+ if (ref->tcache->domainData)
+ {
+ ref->dcc = ref->tcache->domainData;
+ ref->dcc->dccRefCount++;
+ ref->constraints = ref->dcc->constraints;
+ }
+ else
+ ref->constraints = NIL;
+ }
+
+ /*
+ * UpdateDomainConstraintRef --- recheck validity of domain constraint info
+ *
+ * If the domain's constraint set changed, ref->constraints is updated to
+ * point at a new list of cached constraints.
+ *
+ * In the normal case where nothing happened to the domain, this is cheap
+ * enough that it's reasonable (and expected) to check before *each* use
+ * of the constraint info.
+ */
+ void
+ UpdateDomainConstraintRef(DomainConstraintRef *ref)
+ {
+ TypeCacheEntry *typentry = ref->tcache;
+
+ /* Make sure typcache entry's data is up to date */
+ if ((typentry->flags & TCFLAGS_CHECKED_DOMAIN_CONSTRAINTS) == 0 &&
+ typentry->typtype == TYPTYPE_DOMAIN)
+ load_domaintype_info(typentry);
+
+ /* Transfer to ref object if there's new info, adjusting refcounts */
+ if (ref->dcc != typentry->domainData)
+ {
+ /* Paranoia --- be sure link is nulled before trying to release */
+ DomainConstraintCache *dcc = ref->dcc;
+
+ if (dcc)
+ {
+ ref->constraints = NIL;
+ ref->dcc = NULL;
+ decr_dcc_refcount(dcc);
+ }
+ dcc = typentry->domainData;
+ if (dcc)
+ {
+ ref->dcc = dcc;
+ dcc->dccRefCount++;
+ ref->constraints = dcc->constraints;
+ }
+ }
+ }
+
+ /*
+ * DomainHasConstraints --- utility routine to check if a domain has constraints
+ *
+ * This is defined to return false, not fail, if type is not a domain.
+ */
+ bool
+ DomainHasConstraints(Oid type_id)
+ {
+ TypeCacheEntry *typentry;
+
+ /*
+ * Note: a side effect is to cause the typcache's domain data to become
+ * valid. This is fine since we'll likely need it soon if there is any.
+ */
+ typentry = lookup_type_cache(type_id, TYPECACHE_DOMAIN_INFO);
+
+ return (typentry->domainData != NULL);
+ }
+
+
+ /*
* array_element_has_equality and friends are helper routines to check
* whether we should believe that array_eq and related functions will work
* on the given array type or composite type.
*************** TypeCacheOpcCallback(Datum arg, int cach
*** 1003,1008 ****
--- 1368,1407 ----
}
}
+ /*
+ * TypeCacheConstrCallback
+ * Syscache inval callback function
+ *
+ * This is called when a syscache invalidation event occurs for any
+ * pg_constraint or pg_type row. We flush information about domain
+ * constraints when this happens.
+ *
+ * It's slightly annoying that we can't tell whether the inval event was for a
+ * domain constraint/type record or not; there's usually more update traffic
+ * for table constraints/types than domain constraints, so we'll do a lot of
+ * useless flushes. Still, this is better than the old no-caching-at-all
+ * approach to domain constraints.
+ */
+ static void
+ TypeCacheConstrCallback(Datum arg, int cacheid, uint32 hashvalue)
+ {
+ TypeCacheEntry *typentry;
+
+ /*
+ * Because this is called very frequently, and typically very few of the
+ * typcache entries are for domains, we don't use hash_seq_search here.
+ * Instead we thread all the domain-type entries together so that we can
+ * visit them cheaply.
+ */
+ for (typentry = firstDomainTypeEntry;
+ typentry != NULL;
+ typentry = typentry->nextDomain)
+ {
+ /* Reset domain constraint validity information */
+ typentry->flags &= ~TCFLAGS_CHECKED_DOMAIN_CONSTRAINTS;
+ }
+ }
+
/*
* Check if given OID is part of the subset that's sortable by comparisons
diff --git a/src/include/commands/typecmds.h b/src/include/commands/typecmds.h
index e18a714..0a63800 100644
*** a/src/include/commands/typecmds.h
--- b/src/include/commands/typecmds.h
*************** extern Oid AlterDomainDropConstraint(Lis
*** 39,46 ****
extern void checkDomainOwner(HeapTuple tup);
- extern List *GetDomainConstraints(Oid typeOid);
-
extern Oid RenameType(RenameStmt *stmt);
extern Oid AlterTypeOwner(List *names, Oid newOwnerId, ObjectType objecttype);
extern void AlterTypeOwnerInternal(Oid typeOid, Oid newOwnerId,
--- 39,44 ----
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 41288ed..59b17f3 100644
*** a/src/include/nodes/execnodes.h
--- b/src/include/nodes/execnodes.h
*************** typedef struct CoerceToDomainState
*** 942,949 ****
{
ExprState xprstate;
ExprState *arg; /* input expression */
! /* Cached list of constraints that need to be checked */
! List *constraints; /* list of DomainConstraintState nodes */
} CoerceToDomainState;
/*
--- 942,950 ----
{
ExprState xprstate;
ExprState *arg; /* input expression */
! /* Cached set of constraints that need to be checked */
! /* use struct pointer to avoid including typcache.h here */
! struct DomainConstraintRef *constraint_ref;
} CoerceToDomainState;
/*
diff --git a/src/include/utils/typcache.h b/src/include/utils/typcache.h
index b544180..0fe1ce9 100644
*** a/src/include/utils/typcache.h
--- b/src/include/utils/typcache.h
***************
*** 18,25 ****
--- 18,29 ----
#include "access/tupdesc.h"
#include "fmgr.h"
+ #include "nodes/memnodes.h"
+ /* DomainConstraintCache is an opaque struct known only within typcache.c */
+ typedef struct DomainConstraintCache DomainConstraintCache;
+
/* TypeCacheEnumData is an opaque struct known only within typcache.c */
struct TypeCacheEnumData;
*************** typedef struct TypeCacheEntry
*** 84,89 ****
--- 88,99 ----
FmgrInfo rng_canonical_finfo; /* canonicalization function, if any */
FmgrInfo rng_subdiff_finfo; /* difference function, if any */
+ /*
+ * Domain constraint data if it's a domain type. NULL if not domain, or
+ * if domain has no constraints, or if information hasn't been requested.
+ */
+ DomainConstraintCache *domainData;
+
/* Private data, for internal use of typcache.c only */
int flags; /* flags about what we've computed */
*************** typedef struct TypeCacheEntry
*** 92,97 ****
--- 102,110 ----
* information hasn't been requested.
*/
struct TypeCacheEnumData *enumData;
+
+ /* We also maintain a list of all known domain-type cache entries */
+ struct TypeCacheEntry *nextDomain;
} TypeCacheEntry;
/* Bit flags to indicate which fields a given caller needs to have set */
*************** typedef struct TypeCacheEntry
*** 107,115 ****
--- 120,152 ----
#define TYPECACHE_BTREE_OPFAMILY 0x0200
#define TYPECACHE_HASH_OPFAMILY 0x0400
#define TYPECACHE_RANGE_INFO 0x0800
+ #define TYPECACHE_DOMAIN_INFO 0x1000
+
+ /*
+ * Callers wishing to maintain a long-lived reference to a domain's constraint
+ * set must store it in one of these. Use InitDomainConstraintRef() and
+ * UpdateDomainConstraintRef() to manage it. Note: DomainConstraintState is
+ * considered an executable expression type, so it's defined in execnodes.h.
+ */
+ typedef struct DomainConstraintRef
+ {
+ List *constraints; /* list of DomainConstraintState nodes */
+ /* Management data --- treat these fields as private to typcache.c */
+ TypeCacheEntry *tcache; /* owning typcache entry */
+ DomainConstraintCache *dcc; /* current constraints, or NULL if none */
+ MemoryContextCallback callback; /* used to release refcount when done */
+ } DomainConstraintRef;
+
extern TypeCacheEntry *lookup_type_cache(Oid type_id, int flags);
+ extern void InitDomainConstraintRef(Oid type_id, DomainConstraintRef *ref,
+ MemoryContext refctx);
+
+ extern void UpdateDomainConstraintRef(DomainConstraintRef *ref);
+
+ extern bool DomainHasConstraints(Oid type_id);
+
extern TupleDesc lookup_rowtype_tupdesc(Oid type_id, int32 typmod);
extern TupleDesc lookup_rowtype_tupdesc_noerror(Oid type_id, int32 typmod,
diff --git a/src/test/regress/expected/domain.out b/src/test/regress/expected/domain.out
index 78e7704..c107d37 100644
*** a/src/test/regress/expected/domain.out
--- b/src/test/regress/expected/domain.out
*************** ERROR: value for domain orderedpair vio
*** 652,657 ****
--- 652,687 ----
CONTEXT: PL/pgSQL function array_elem_check(integer) line 5 at assignment
drop function array_elem_check(int);
--
+ -- Check enforcement of changing constraints in plpgsql
+ --
+ create domain di as int;
+ create function dom_check(int) returns di as $$
+ declare d di;
+ begin
+ d := $1;
+ return d;
+ end
+ $$ language plpgsql immutable;
+ select dom_check(0);
+ dom_check
+ -----------
+ 0
+ (1 row)
+
+ alter domain di add constraint pos check (value > 0);
+ select dom_check(0); -- fail
+ ERROR: value for domain di violates check constraint "pos"
+ CONTEXT: PL/pgSQL function dom_check(integer) line 4 at assignment
+ alter domain di drop constraint pos;
+ select dom_check(0);
+ dom_check
+ -----------
+ 0
+ (1 row)
+
+ drop function dom_check(int);
+ drop domain di;
+ --
-- Renaming
--
create domain testdomain1 as int;
diff --git a/src/test/regress/sql/domain.sql b/src/test/regress/sql/domain.sql
index 5af36af..ab1fcd3 100644
*** a/src/test/regress/sql/domain.sql
--- b/src/test/regress/sql/domain.sql
*************** select array_elem_check(-1);
*** 487,492 ****
--- 487,519 ----
drop function array_elem_check(int);
+ --
+ -- Check enforcement of changing constraints in plpgsql
+ --
+
+ create domain di as int;
+
+ create function dom_check(int) returns di as $$
+ declare d di;
+ begin
+ d := $1;
+ return d;
+ end
+ $$ language plpgsql immutable;
+
+ select dom_check(0);
+
+ alter domain di add constraint pos check (value > 0);
+
+ select dom_check(0); -- fail
+
+ alter domain di drop constraint pos;
+
+ select dom_check(0);
+
+ drop function dom_check(int);
+
+ drop domain di;
--
-- Renaming
>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes: Tom> This is the first attempt at weaponizing the memory contextTom> reset/delete feature, and I'm fairly happy with it,except for oneTom> thing: I had to #include utils/memnodes.h into typcache.h in orderTom> to preserve the intended propertythat the callback structs couldTom> be included directly into structs using them. Now, that's notTom> awful in itself,because typcache.h isn't used everywhere; but ifTom> this feature gets popular we'll find memnodes.h being includedTom>pretty much everywhere, which is not so great. I'm thinking aboutTom> moving struct MemoryContextCallback andthe extern forTom> MemoryContextRegisterResetCallback into palloc.h to avoid this.Tom> Maybe that's too far in the otherdirection. Thoughts?Tom> Compromise ideas? This was pretty much my first thought on looking at the callback patch. It may seem logical to put the reset callback stuff in memutils/memnodes alongside context creation and reset and so on, but in fact the places that are going to want to use callbacks are primarily _not_ the places that are doing their own context management - since those could do their own cleanup - but rather places that are allocating things in contexts supplied by others. So palloc.h is the place for it. -- Andrew (irc:RhodiumToad)
Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
> Tom> This is the first attempt at weaponizing the memory context
> Tom> reset/delete feature, and I'm fairly happy with it, except for one
> Tom> thing: I had to #include utils/memnodes.h into typcache.h in order
> Tom> to preserve the intended property that the callback structs could
> Tom> be included directly into structs using them. Now, that's not
> Tom> awful in itself, because typcache.h isn't used everywhere; but if
> Tom> this feature gets popular we'll find memnodes.h being included
> Tom> pretty much everywhere, which is not so great. I'm thinking about
> Tom> moving struct MemoryContextCallback and the extern for
> Tom> MemoryContextRegisterResetCallback into palloc.h to avoid this.
> Tom> Maybe that's too far in the other direction. Thoughts?
> Tom> Compromise ideas?
> This was pretty much my first thought on looking at the callback
> patch. It may seem logical to put the reset callback stuff in
> memutils/memnodes alongside context creation and reset and so on, but in
> fact the places that are going to want to use callbacks are primarily
> _not_ the places that are doing their own context management - since
> those could do their own cleanup - but rather places that are allocating
> things in contexts supplied by others. So palloc.h is the place for it.
That argument sounds good to me ;-). Moved.
regards, tom lane
On 2/28/15 11:26 PM, Tom Lane wrote: > Also, instrumenting the code showed that TypeCacheConstrCallback gets > called quite a lot during the standard regression tests, which is why > I went out of my way to make it quick. Almost all of those cache flushes > are from non-domain-related updates on pg_type or pg_constraint, so > they're not really necessary from a logical perspective, and they're > surely going to hurt performance for heavy users of domains. I think to > fix this we'd have to split pg_constraint into two catalogs, one for table > constraints and one for domain constraints; which would be a good thing > anyway from a normal-form-theory perspective. And we'd have to get rid of > pg_type.typnotnull and instead store domain NOT NULL constraints in this > hypothetical domain constraint catalog. I don't plan to do anything about > that myself right now, because I'm not sure that production databases > would have the kind of traffic on pg_type and pg_constraint that the > regression tests exhibit. But at some point we might have to fix it. FWIW, my experience running a low-downtime website and supporting DDL during normal operations (ie: no maintenance windows) is that by far the biggest concern is acquiring locks. Once you have the locks, taking an extra second for the actual DDL isn't that big a deal (and I suspect you'd need to do a LOT of DDL to add up to that). Likewise, after piling up waiting for a DDL lock to release, I really doubt the extra sinval workload is going to matter much. If you're pushing the hardware that hard I doubt you'd be able to do online DDL for a slew of other reasons. -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.com
I wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Thu, Feb 26, 2015 at 1:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> To some extent this is a workaround for the fact that plpgsql does type
>>> conversions the way it does (ie, by I/O rather than by using the parser's
>>> cast mechanisms). We've talked about changing that, but people seem to
>>> be afraid of the compatibility consequences, and I'm not planning to fight
>>> two compatibility battles concurrently ;-)
>> A sensible plan, but since we're here:
>> - I do agree that changing the way PL/pgsql does those type
>> conversions is scary. I can't predict what will break, and there's no
>> getting around the scariness of that.
>> - On the other hand, I do think it would be good to change it, because
>> this sucks:
>> rhaas=# do $$ declare x int; begin x := (3.0::numeric)/(1.0::numeric); end $$;
>> ERROR: invalid input syntax for integer: "3.0000000000000000"
>> CONTEXT: PL/pgSQL function inline_code_block line 1 at assignment
> If we did want to open that can of worms, my proposal would be to make
> plpgsql type conversions work like so:
> * If there is a cast pathway known to the core SQL parser, use that
> mechanism.
> * Otherwise, attempt to convert via I/O as we do today.
> This seems to minimize the risk of breaking things, although there would
> probably be corner cases that work differently (for instance I bet boolean
> to text might turn out differently). In the very long run we could perhaps
> deprecate and remove the second phase.
Since people didn't seem to be running away screaming, here is a patch to
do that. I've gone through the list of existing casts, and as far as I
can tell the only change in behavior in cases that would have worked
before is the aforementioned boolean->string case. Before, if you made
plpgsql convert a bool to text, you got 't' or 'f', eg
regression=# do $$declare t text; begin t := true; raise notice 't = %', t; end $$;
NOTICE: t = t
DO
Now you get 'true' or 'false', because that's what the cast does, eg
regression=# do $$declare t text; begin t := true; raise notice 't = %', t; end $$;
NOTICE: t = true
DO
This seems to me to be a narrow enough behavioral change that we could
tolerate it in a major release. (Of course, maybe somebody out there
thinks that failures like the one Robert exhibits are a feature, in
which case they'd have a rather longer list of compatibility issues.)
The performance gain is fairly nifty. I tested int->bigint coercions
like this:
do $$
declare b bigint;
begin
for i in 1 .. 1000000 loop
b := i;
end loop;
end$$;
On my machine, in non-cassert builds, this takes around 750 ms in 9.4,
610 ms in HEAD (so we already did something good, I'm not quite sure
what), and 420 ms with this patch. Another example is a no-op domain
conversion:
create domain di int;
do $$
declare b di;
begin
for i in 1 .. 1000000 loop
b := i;
end loop;
end$$;
This takes about 760 ms in 9.4, 660 ms in HEAD, 380 ms with patch.
Comments?
regards, tom lane
diff --git a/src/pl/plpgsql/src/pl_comp.c b/src/pl/plpgsql/src/pl_comp.c
index f364ce4..1621254 100644
*** a/src/pl/plpgsql/src/pl_comp.c
--- b/src/pl/plpgsql/src/pl_comp.c
*************** do_compile(FunctionCallInfo fcinfo,
*** 559,566 ****
{
function->fn_retbyval = typeStruct->typbyval;
function->fn_rettyplen = typeStruct->typlen;
- function->fn_rettypioparam = getTypeIOParam(typeTup);
- fmgr_info(typeStruct->typinput, &(function->fn_retinput));
/*
* install $0 reference, but only for polymorphic return
--- 559,564 ----
*************** plpgsql_compile_inline(char *proc_source
*** 803,809 ****
char *func_name = "inline_code_block";
PLpgSQL_function *function;
ErrorContextCallback plerrcontext;
- Oid typinput;
PLpgSQL_variable *var;
int parse_rc;
MemoryContext func_cxt;
--- 801,806 ----
*************** plpgsql_compile_inline(char *proc_source
*** 876,883 ****
/* a bit of hardwired knowledge about type VOID here */
function->fn_retbyval = true;
function->fn_rettyplen = sizeof(int32);
- getTypeInputInfo(VOIDOID, &typinput, &function->fn_rettypioparam);
- fmgr_info(typinput, &(function->fn_retinput));
/*
* Remember if function is STABLE/IMMUTABLE. XXX would it be better to
--- 873,878 ----
*************** build_datatype(HeapTuple typeTup, int32
*** 2200,2211 ****
}
typ->typlen = typeStruct->typlen;
typ->typbyval = typeStruct->typbyval;
typ->typrelid = typeStruct->typrelid;
- typ->typioparam = getTypeIOParam(typeTup);
typ->collation = typeStruct->typcollation;
if (OidIsValid(collation) && OidIsValid(typ->collation))
typ->collation = collation;
- fmgr_info(typeStruct->typinput, &(typ->typinput));
typ->atttypmod = typmod;
return typ;
--- 2195,2205 ----
}
typ->typlen = typeStruct->typlen;
typ->typbyval = typeStruct->typbyval;
+ typ->typisdomain = (typeStruct->typtype == TYPTYPE_DOMAIN);
typ->typrelid = typeStruct->typrelid;
typ->collation = typeStruct->typcollation;
if (OidIsValid(collation) && OidIsValid(typ->collation))
typ->collation = collation;
typ->atttypmod = typmod;
return typ;
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 41a68f8..4b4a442 100644
*** a/src/pl/plpgsql/src/pl_exec.c
--- b/src/pl/plpgsql/src/pl_exec.c
***************
*** 26,31 ****
--- 26,33 ----
#include "funcapi.h"
#include "miscadmin.h"
#include "nodes/nodeFuncs.h"
+ #include "optimizer/planner.h"
+ #include "parser/parse_coerce.h"
#include "parser/scansup.h"
#include "storage/proc.h"
#include "tcop/tcopprot.h"
*************** typedef struct
*** 50,55 ****
--- 52,71 ----
bool *freevals; /* which arguments are pfree-able */
} PreparedParamsData;
+ typedef struct
+ {
+ /* NB: we assume this struct contains no padding bytes */
+ Oid srctype; /* source type for cast */
+ Oid dsttype; /* destination type for cast */
+ int32 dsttypmod; /* destination typmod for cast */
+ } plpgsql_CastHashKey;
+
+ typedef struct
+ {
+ plpgsql_CastHashKey key; /* hash key --- MUST BE FIRST */
+ ExprState *cast_exprstate; /* cast expression, or NULL if no-op cast */
+ } plpgsql_CastHashEntry;
+
/*
* All plpgsql function executions within a single transaction share the same
* executor EState for evaluating "simple" expressions. Each function call
*************** static void exec_move_row_from_datum(PLp
*** 211,225 ****
static char *convert_value_to_string(PLpgSQL_execstate *estate,
Datum value, Oid valtype);
static Datum exec_cast_value(PLpgSQL_execstate *estate,
! Datum value, bool isnull,
Oid valtype, int32 valtypmod,
! Oid reqtype, int32 reqtypmod,
! FmgrInfo *reqinput,
! Oid reqtypioparam);
! static Datum exec_simple_cast_value(PLpgSQL_execstate *estate,
! Datum value, bool isnull,
! Oid valtype, int32 valtypmod,
! Oid reqtype, int32 reqtypmod);
static void exec_init_tuple_store(PLpgSQL_execstate *estate);
static void exec_set_found(PLpgSQL_execstate *estate, bool state);
static void plpgsql_create_econtext(PLpgSQL_execstate *estate);
--- 227,237 ----
static char *convert_value_to_string(PLpgSQL_execstate *estate,
Datum value, Oid valtype);
static Datum exec_cast_value(PLpgSQL_execstate *estate,
! Datum value, bool *isnull,
Oid valtype, int32 valtypmod,
! Oid reqtype, int32 reqtypmod);
! static ExprState *get_cast_expression(PLpgSQL_execstate *estate,
! Oid srctype, Oid dsttype, int32 dsttypmod);
static void exec_init_tuple_store(PLpgSQL_execstate *estate);
static void exec_set_found(PLpgSQL_execstate *estate, bool state);
static void plpgsql_create_econtext(PLpgSQL_execstate *estate);
*************** plpgsql_exec_function(PLpgSQL_function *
*** 454,466 ****
/* Cast value to proper type */
estate.retval = exec_cast_value(&estate,
estate.retval,
! fcinfo->isnull,
estate.rettype,
-1,
func->fn_rettype,
! -1,
! &(func->fn_retinput),
! func->fn_rettypioparam);
/*
* If the function's return type isn't by value, copy the value
--- 466,476 ----
/* Cast value to proper type */
estate.retval = exec_cast_value(&estate,
estate.retval,
! &fcinfo->isnull,
estate.rettype,
-1,
func->fn_rettype,
! -1);
/*
* If the function's return type isn't by value, copy the value
*************** exec_stmt_block(PLpgSQL_execstate *estat
*** 1079,1085 ****
* before the notnull check to be consistent with
* exec_assign_value.)
*/
! if (!var->datatype->typinput.fn_strict)
exec_assign_value(estate,
(PLpgSQL_datum *) var,
(Datum) 0,
--- 1089,1095 ----
* before the notnull check to be consistent with
* exec_assign_value.)
*/
! if (var->datatype->typisdomain)
exec_assign_value(estate,
(PLpgSQL_datum *) var,
(Datum) 0,
*************** exec_stmt_fori(PLpgSQL_execstate *estate
*** 1903,1914 ****
*/
value = exec_eval_expr(estate, stmt->lower,
&isnull, &valtype, &valtypmod);
! value = exec_cast_value(estate, value, isnull,
valtype, valtypmod,
var->datatype->typoid,
! var->datatype->atttypmod,
! &(var->datatype->typinput),
! var->datatype->typioparam);
if (isnull)
ereport(ERROR,
(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
--- 1913,1922 ----
*/
value = exec_eval_expr(estate, stmt->lower,
&isnull, &valtype, &valtypmod);
! value = exec_cast_value(estate, value, &isnull,
valtype, valtypmod,
var->datatype->typoid,
! var->datatype->atttypmod);
if (isnull)
ereport(ERROR,
(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
*************** exec_stmt_fori(PLpgSQL_execstate *estate
*** 1921,1932 ****
*/
value = exec_eval_expr(estate, stmt->upper,
&isnull, &valtype, &valtypmod);
! value = exec_cast_value(estate, value, isnull,
valtype, valtypmod,
var->datatype->typoid,
! var->datatype->atttypmod,
! &(var->datatype->typinput),
! var->datatype->typioparam);
if (isnull)
ereport(ERROR,
(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
--- 1929,1938 ----
*/
value = exec_eval_expr(estate, stmt->upper,
&isnull, &valtype, &valtypmod);
! value = exec_cast_value(estate, value, &isnull,
valtype, valtypmod,
var->datatype->typoid,
! var->datatype->atttypmod);
if (isnull)
ereport(ERROR,
(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
*************** exec_stmt_fori(PLpgSQL_execstate *estate
*** 1941,1952 ****
{
value = exec_eval_expr(estate, stmt->step,
&isnull, &valtype, &valtypmod);
! value = exec_cast_value(estate, value, isnull,
valtype, valtypmod,
var->datatype->typoid,
! var->datatype->atttypmod,
! &(var->datatype->typinput),
! var->datatype->typioparam);
if (isnull)
ereport(ERROR,
(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
--- 1947,1956 ----
{
value = exec_eval_expr(estate, stmt->step,
&isnull, &valtype, &valtypmod);
! value = exec_cast_value(estate, value, &isnull,
valtype, valtypmod,
var->datatype->typoid,
! var->datatype->atttypmod);
if (isnull)
ereport(ERROR,
(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
*************** exec_stmt_return_next(PLpgSQL_execstate
*** 2614,2626 ****
errmsg("wrong result type supplied in RETURN NEXT")));
/* coerce type if needed */
! retval = exec_simple_cast_value(estate,
! retval,
! isNull,
! var->datatype->typoid,
! var->datatype->atttypmod,
! tupdesc->attrs[0]->atttypid,
! tupdesc->attrs[0]->atttypmod);
tuplestore_putvalues(estate->tuple_store, tupdesc,
&retval, &isNull);
--- 2618,2630 ----
errmsg("wrong result type supplied in RETURN NEXT")));
/* coerce type if needed */
! retval = exec_cast_value(estate,
! retval,
! &isNull,
! var->datatype->typoid,
! var->datatype->atttypmod,
! tupdesc->attrs[0]->atttypid,
! tupdesc->attrs[0]->atttypmod);
tuplestore_putvalues(estate->tuple_store, tupdesc,
&retval, &isNull);
*************** exec_stmt_return_next(PLpgSQL_execstate
*** 2740,2752 ****
errmsg("wrong result type supplied in RETURN NEXT")));
/* coerce type if needed */
! retval = exec_simple_cast_value(estate,
! retval,
! isNull,
! rettype,
! rettypmod,
! tupdesc->attrs[0]->atttypid,
! tupdesc->attrs[0]->atttypmod);
tuplestore_putvalues(estate->tuple_store, tupdesc,
&retval, &isNull);
--- 2744,2756 ----
errmsg("wrong result type supplied in RETURN NEXT")));
/* coerce type if needed */
! retval = exec_cast_value(estate,
! retval,
! &isNull,
! rettype,
! rettypmod,
! tupdesc->attrs[0]->atttypid,
! tupdesc->attrs[0]->atttypmod);
tuplestore_putvalues(estate->tuple_store, tupdesc,
&retval, &isNull);
*************** exec_assign_value(PLpgSQL_execstate *est
*** 4070,4082 ****
newvalue = exec_cast_value(estate,
value,
! isNull,
valtype,
valtypmod,
var->datatype->typoid,
! var->datatype->atttypmod,
! &(var->datatype->typinput),
! var->datatype->typioparam);
if (isNull && var->notnull)
ereport(ERROR,
--- 4074,4084 ----
newvalue = exec_cast_value(estate,
value,
! &isNull,
valtype,
valtypmod,
var->datatype->typoid,
! var->datatype->atttypmod);
if (isNull && var->notnull)
ereport(ERROR,
*************** exec_assign_value(PLpgSQL_execstate *est
*** 4220,4232 ****
*/
atttype = rec->tupdesc->attrs[fno]->atttypid;
atttypmod = rec->tupdesc->attrs[fno]->atttypmod;
! values[fno] = exec_simple_cast_value(estate,
! value,
! isNull,
! valtype,
! valtypmod,
! atttype,
! atttypmod);
nulls[fno] = isNull;
/*
--- 4222,4234 ----
*/
atttype = rec->tupdesc->attrs[fno]->atttypid;
atttypmod = rec->tupdesc->attrs[fno]->atttypmod;
! values[fno] = exec_cast_value(estate,
! value,
! &isNull,
! valtype,
! valtypmod,
! atttype,
! atttypmod);
nulls[fno] = isNull;
/*
*************** exec_assign_value(PLpgSQL_execstate *est
*** 4383,4395 ****
estate->eval_tuptable = save_eval_tuptable;
/* Coerce source value to match array element type. */
! coerced_value = exec_simple_cast_value(estate,
! value,
! isNull,
! valtype,
! valtypmod,
! arrayelem->elemtypoid,
! arrayelem->arraytypmod);
/*
* If the original array is null, cons up an empty array so
--- 4385,4397 ----
estate->eval_tuptable = save_eval_tuptable;
/* Coerce source value to match array element type. */
! coerced_value = exec_cast_value(estate,
! value,
! &isNull,
! valtype,
! valtypmod,
! arrayelem->elemtypoid,
! arrayelem->arraytypmod);
/*
* If the original array is null, cons up an empty array so
*************** exec_eval_integer(PLpgSQL_execstate *est
*** 4760,4768 ****
int32 exprtypmod;
exprdatum = exec_eval_expr(estate, expr, isNull, &exprtypeid, &exprtypmod);
! exprdatum = exec_simple_cast_value(estate, exprdatum, *isNull,
! exprtypeid, exprtypmod,
! INT4OID, -1);
return DatumGetInt32(exprdatum);
}
--- 4762,4770 ----
int32 exprtypmod;
exprdatum = exec_eval_expr(estate, expr, isNull, &exprtypeid, &exprtypmod);
! exprdatum = exec_cast_value(estate, exprdatum, isNull,
! exprtypeid, exprtypmod,
! INT4OID, -1);
return DatumGetInt32(exprdatum);
}
*************** exec_eval_boolean(PLpgSQL_execstate *est
*** 4783,4791 ****
int32 exprtypmod;
exprdatum = exec_eval_expr(estate, expr, isNull, &exprtypeid, &exprtypmod);
! exprdatum = exec_simple_cast_value(estate, exprdatum, *isNull,
! exprtypeid, exprtypmod,
! BOOLOID, -1);
return DatumGetBool(exprdatum);
}
--- 4785,4793 ----
int32 exprtypmod;
exprdatum = exec_eval_expr(estate, expr, isNull, &exprtypeid, &exprtypmod);
! exprdatum = exec_cast_value(estate, exprdatum, isNull,
! exprtypeid, exprtypmod,
! BOOLOID, -1);
return DatumGetBool(exprdatum);
}
*************** convert_value_to_string(PLpgSQL_execstat
*** 5705,5710 ****
--- 5707,5716 ----
/* ----------
* exec_cast_value Cast a value if required
*
+ * Note that *isnull is an input and also an output parameter. While it's
+ * unlikely that a cast operation would produce null from non-null or vice
+ * versa, that could in principle happen.
+ *
* Note: the estate's eval_econtext is used for temporary storage, and may
* also contain the result Datum if we have to do a conversion to a pass-
* by-reference data type. Be sure to do an exec_eval_cleanup() call when
*************** convert_value_to_string(PLpgSQL_execstat
*** 5713,5723 ****
*/
static Datum
exec_cast_value(PLpgSQL_execstate *estate,
! Datum value, bool isnull,
Oid valtype, int32 valtypmod,
! Oid reqtype, int32 reqtypmod,
! FmgrInfo *reqinput,
! Oid reqtypioparam)
{
/*
* If the type of the given value isn't what's requested, convert it.
--- 5719,5727 ----
*/
static Datum
exec_cast_value(PLpgSQL_execstate *estate,
! Datum value, bool *isnull,
Oid valtype, int32 valtypmod,
! Oid reqtype, int32 reqtypmod)
{
/*
* If the type of the given value isn't what's requested, convert it.
*************** exec_cast_value(PLpgSQL_execstate *estat
*** 5725,5791 ****
if (valtype != reqtype ||
(valtypmod != reqtypmod && reqtypmod != -1))
{
! MemoryContext oldcontext;
! oldcontext = MemoryContextSwitchTo(estate->eval_econtext->ecxt_per_tuple_memory);
! if (!isnull)
{
! char *extval;
! extval = convert_value_to_string(estate, value, valtype);
! value = InputFunctionCall(reqinput, extval,
! reqtypioparam, reqtypmod);
! }
! else
! {
! value = InputFunctionCall(reqinput, NULL,
! reqtypioparam, reqtypmod);
}
- MemoryContextSwitchTo(oldcontext);
}
return value;
}
/* ----------
! * exec_simple_cast_value Cast a value if required
*
! * As above, but need not supply details about target type. Note that this
! * is slower than exec_cast_value with cached type info, and so should be
! * avoided in heavily used code paths.
* ----------
*/
! static Datum
! exec_simple_cast_value(PLpgSQL_execstate *estate,
! Datum value, bool isnull,
! Oid valtype, int32 valtypmod,
! Oid reqtype, int32 reqtypmod)
{
! if (valtype != reqtype ||
! (valtypmod != reqtypmod && reqtypmod != -1))
{
! Oid typinput;
! Oid typioparam;
! FmgrInfo finfo_input;
! getTypeInputInfo(reqtype, &typinput, &typioparam);
! fmgr_info(typinput, &finfo_input);
! value = exec_cast_value(estate,
! value,
! isnull,
! valtype,
! valtypmod,
! reqtype,
! reqtypmod,
! &finfo_input,
! typioparam);
}
! return value;
! }
/* ----------
* exec_simple_check_node - Recursively check if an expression
--- 5729,5899 ----
if (valtype != reqtype ||
(valtypmod != reqtypmod && reqtypmod != -1))
{
! ExprState *cast_expr;
! cast_expr = get_cast_expression(estate, valtype, reqtype, reqtypmod);
! if (cast_expr)
{
! ExprContext *econtext = estate->eval_econtext;
! MemoryContext oldcontext;
! oldcontext = MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory);
! econtext->caseValue_datum = value;
! econtext->caseValue_isNull = *isnull;
!
! value = ExecEvalExpr(cast_expr, econtext, isnull, NULL);
!
! MemoryContextSwitchTo(oldcontext);
}
}
return value;
}
/* ----------
! * get_cast_expression Look up how to perform a type cast
*
! * Returns an expression evaluation tree based on a CaseTestExpr input,
! * or NULL if the cast is a mere no-op relabeling.
! *
! * We cache the results of the lookup in a per-function hash table.
! * In principle this could probably be a session-wide hash table instead,
! * but that introduces some corner-case questions that probably aren't
! * worth dealing with; in particular that re-entrant use of an evaluation
! * tree might occur.
* ----------
*/
! static ExprState *
! get_cast_expression(PLpgSQL_execstate *estate,
! Oid srctype, Oid dsttype, int32 dsttypmod)
{
! HTAB *cast_hash = estate->func->cast_hash;
! plpgsql_CastHashKey cast_key;
! plpgsql_CastHashEntry *cast_entry;
! bool found;
! CaseTestExpr *placeholder;
! Node *cast_expr;
! ExprState *cast_exprstate;
! MemoryContext oldcontext;
!
! /* Create the cast-info hash table if we didn't already */
! if (cast_hash == NULL)
{
! HASHCTL ctl;
! memset(&ctl, 0, sizeof(ctl));
! ctl.keysize = sizeof(plpgsql_CastHashKey);
! ctl.entrysize = sizeof(plpgsql_CastHashEntry);
! ctl.hcxt = estate->func->fn_cxt;
! cast_hash = hash_create("PLpgSQL cast cache",
! 16, /* start small and extend */
! &ctl,
! HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);
! estate->func->cast_hash = cast_hash;
! }
! /* Look for existing entry */
! cast_key.srctype = srctype;
! cast_key.dsttype = dsttype;
! cast_key.dsttypmod = dsttypmod;
! cast_entry = (plpgsql_CastHashEntry *) hash_search(cast_hash,
! (void *) &cast_key,
! HASH_FIND, NULL);
! if (cast_entry)
! return cast_entry->cast_exprstate;
! /* Construct expression tree for coercion in function's context */
! oldcontext = MemoryContextSwitchTo(estate->func->fn_cxt);
!
! /*
! * We use a CaseTestExpr as the base of the coercion tree, since it's very
! * cheap to insert the source value for that.
! */
! placeholder = makeNode(CaseTestExpr);
! placeholder->typeId = srctype;
! placeholder->typeMod = -1;
! placeholder->collation = get_typcollation(srctype);
! if (OidIsValid(estate->func->fn_input_collation) &&
! OidIsValid(placeholder->collation))
! placeholder->collation = estate->func->fn_input_collation;
!
! /*
! * Apply coercion. We use ASSIGNMENT coercion because that's the closest
! * match to plpgsql's historical behavior; in particular, EXPLICIT
! * coercion would allow silent truncation to a destination
! * varchar/bpchar's length, which we do not want.
! *
! * If source type is UNKNOWN, coerce_to_target_type will fail (it only
! * expects to see that for Const input nodes), so don't call it; we'll
! * apply CoerceViaIO instead.
! */
! if (srctype != UNKNOWNOID)
! cast_expr = coerce_to_target_type(NULL,
! (Node *) placeholder, srctype,
! dsttype, dsttypmod,
! COERCION_ASSIGNMENT,
! COERCE_IMPLICIT_CAST,
! -1);
! else
! cast_expr = NULL;
!
! /*
! * If there's no cast path according to the parser, fall back to using an
! * I/O coercion; this is semantically dubious but matches plpgsql's
! * historical behavior.
! */
! if (cast_expr == NULL)
! {
! CoerceViaIO *iocoerce = makeNode(CoerceViaIO);
!
! iocoerce->arg = (Expr *) placeholder;
! iocoerce->resulttype = dsttype;
! iocoerce->resultcollid = InvalidOid;
! iocoerce->coerceformat = COERCE_IMPLICIT_CAST;
! iocoerce->location = -1;
! cast_expr = (Node *) iocoerce;
! if (dsttypmod != -1)
! cast_expr = coerce_to_target_type(NULL,
! cast_expr, dsttype,
! dsttype, dsttypmod,
! COERCION_ASSIGNMENT,
! COERCE_IMPLICIT_CAST,
! -1);
}
! /* Note: we don't bother labeling the expression tree with collation */
!
! /* Detect whether we have a no-op (RelabelType) coercion */
! if (IsA(cast_expr, RelabelType) &&
! ((RelabelType *) cast_expr)->arg == (Expr *) placeholder)
! cast_expr = NULL;
!
! if (cast_expr)
! {
! /* ExecInitExpr assumes we've planned the expression */
! cast_expr = (Node *) expression_planner((Expr *) cast_expr);
! /* Create an expression eval state tree for it */
! cast_exprstate = ExecInitExpr((Expr *) cast_expr, NULL);
! }
! else
! cast_exprstate = NULL;
!
! MemoryContextSwitchTo(oldcontext);
!
! /*
! * Now fill in a hashtable entry. If we fail anywhere up to/including
! * this step, we've only leaked some memory in the function context, which
! * isn't great but isn't disastrous either.
! */
! cast_entry = (plpgsql_CastHashEntry *) hash_search(cast_hash,
! (void *) &cast_key,
! HASH_ENTER, &found);
! Assert(!found); /* wasn't there a moment ago */
!
! cast_entry->cast_exprstate = cast_exprstate;
+ return cast_exprstate;
+ }
/* ----------
* exec_simple_check_node - Recursively check if an expression
diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h
index 624c91e..8bac860 100644
*** a/src/pl/plpgsql/src/plpgsql.h
--- b/src/pl/plpgsql/src/plpgsql.h
***************
*** 22,27 ****
--- 22,28 ----
#include "commands/event_trigger.h"
#include "commands/trigger.h"
#include "executor/spi.h"
+ #include "utils/hsearch.h"
/**********************************************************************
* Definitions
*************** typedef struct
*** 178,187 ****
int ttype; /* PLPGSQL_TTYPE_ code */
int16 typlen; /* stuff copied from its pg_type entry */
bool typbyval;
Oid typrelid;
- Oid typioparam;
Oid collation; /* from pg_type, but can be overridden */
- FmgrInfo typinput; /* lookup info for typinput function */
int32 atttypmod; /* typmod (taken from someplace else) */
} PLpgSQL_type;
--- 179,187 ----
int ttype; /* PLPGSQL_TTYPE_ code */
int16 typlen; /* stuff copied from its pg_type entry */
bool typbyval;
+ bool typisdomain;
Oid typrelid;
Oid collation; /* from pg_type, but can be overridden */
int32 atttypmod; /* typmod (taken from someplace else) */
} PLpgSQL_type;
*************** typedef struct PLpgSQL_function
*** 709,716 ****
Oid fn_rettype;
int fn_rettyplen;
bool fn_retbyval;
- FmgrInfo fn_retinput;
- Oid fn_rettypioparam;
bool fn_retistuple;
bool fn_retset;
bool fn_readonly;
--- 709,714 ----
*************** typedef struct PLpgSQL_function
*** 748,753 ****
--- 746,754 ----
PLpgSQL_datum **datums;
PLpgSQL_stmt_block *action;
+ /* table for performing casts needed in this function */
+ HTAB *cast_hash;
+
/* these fields change when the function is used */
struct PLpgSQL_execstate *cur_estate;
unsigned long use_count;
2015-03-03 20:32 GMT+01:00 Tom Lane <tgl@sss.pgh.pa.us>:
I wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Thu, Feb 26, 2015 at 1:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> To some extent this is a workaround for the fact that plpgsql does type
>>> conversions the way it does (ie, by I/O rather than by using the parser's
>>> cast mechanisms). We've talked about changing that, but people seem to
>>> be afraid of the compatibility consequences, and I'm not planning to fight
>>> two compatibility battles concurrently ;-)
>> A sensible plan, but since we're here:
>> - I do agree that changing the way PL/pgsql does those type
>> conversions is scary. I can't predict what will break, and there's no
>> getting around the scariness of that.
>> - On the other hand, I do think it would be good to change it, because
>> this sucks:
>> rhaas=# do $$ declare x int; begin x := (3.0::numeric)/(1.0::numeric); end $$;
>> ERROR: invalid input syntax for integer: "3.0000000000000000"
>> CONTEXT: PL/pgSQL function inline_code_block line 1 at assignment
> If we did want to open that can of worms, my proposal would be to make
> plpgsql type conversions work like so:
> * If there is a cast pathway known to the core SQL parser, use that
> mechanism.
> * Otherwise, attempt to convert via I/O as we do today.
> This seems to minimize the risk of breaking things, although there would
> probably be corner cases that work differently (for instance I bet boolean
> to text might turn out differently). In the very long run we could perhaps
> deprecate and remove the second phase.
Since people didn't seem to be running away screaming, here is a patch to
do that. I've gone through the list of existing casts, and as far as I
can tell the only change in behavior in cases that would have worked
before is the aforementioned boolean->string case. Before, if you made
plpgsql convert a bool to text, you got 't' or 'f', eg
regression=# do $$declare t text; begin t := true; raise notice 't = %', t; end $$;
NOTICE: t = t
DO
Now you get 'true' or 'false', because that's what the cast does, eg
regression=# do $$declare t text; begin t := true; raise notice 't = %', t; end $$;
NOTICE: t = true
DO
This seems to me to be a narrow enough behavioral change that we could
tolerate it in a major release. (Of course, maybe somebody out there
thinks that failures like the one Robert exhibits are a feature, in
which case they'd have a rather longer list of compatibility issues.)
The performance gain is fairly nifty. I tested int->bigint coercions
like this:
do $$
declare b bigint;
begin
for i in 1 .. 1000000 loop
b := i;
end loop;
end$$;
On my machine, in non-cassert builds, this takes around 750 ms in 9.4,
610 ms in HEAD (so we already did something good, I'm not quite sure
what), and 420 ms with this patch. Another example is a no-op domain
conversion:
create domain di int;
do $$
declare b di;
begin
for i in 1 .. 1000000 loop
b := i;
end loop;
end$$;
This takes about 760 ms in 9.4, 660 ms in HEAD, 380 ms with patch.
Comments?
it is perfect,
thank you very much for this
Regards
Pavel Stehule
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