Kevin Grittner <kgri...@ymail.com> wrote:
> I'll leave this alone for a day. If nobody objects, I will change
> the ruleutils.c code to work with either value (to support
> pg_upgrade) and change the code to set this to zero, for 9.3 and
> forward only. I will change the 9.3 docs to mention that newly
> created rows will have zero, but that clusters upgraded via
> pg_upgrade may still have some rows containing the old value of -1.
>
> For anyone casually following along without reading the code, it's
> not a bug in the sense that current code ever misbehaves, but the
> value which has been used for ev_attr to indicate "whole table"
> since at least Postgres95 version 1.01 is not consistent with other
> places we set a dummy value in attribute number to indicate "whole
> table". Since 2002 we have not supported column-level rules, so it
> has just been filled with a constant of -1. The idea is to change
> the constant to zero -- to make it more consistent so as to reduce
> confusion and the chance of future bugs should we ever decide to
> use the column again.
When I went to make this change, I found over 100 lines of obsolete
code related to this. Commit 95ef6a344821655ce4d0a74999ac49dd6af6d342
removed the ability to create a rule on a column effective with
7.3, but a lot of code for supporting that was left behind. It
seems to me that the rewrite area is complicated without carrying
code that's been dead for over a decade. The first patch removes
the dead weight. The second patch makes the change suggested by
Tom. Those carrying forward from beta1 without an initdb will
still see some rows in pg_rewrite with -1, but anyone else will see
zero for this column.
Does anyone see a problem with applying both of these for 9.3?
> If we were a little earlier in the release cycle I would argue that
> if we're going to do anything with this column we should drop it.
Which is exactly what I think we should do as soon as we branch.
--
Kevin Grittner
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
*** a/src/backend/rewrite/rewriteHandler.c
--- b/src/backend/rewrite/rewriteHandler.c
***************
*** 1274,1288 **** matchLocks(CmdType event,
}
}
! if (oneLock->event == event)
! {
! if (parsetree->commandType != CMD_SELECT ||
! (oneLock->attrno == -1 ?
! rangeTableEntry_used((Node *) parsetree, varno, 0) :
! attribute_used((Node *) parsetree,
! varno, oneLock->attrno, 0)))
! matching_locks = lappend(matching_locks, oneLock);
! }
}
return matching_locks;
--- 1274,1281 ----
}
}
! if (oneLock->event == event && parsetree->commandType != CMD_SELECT)
! matching_locks = lappend(matching_locks, oneLock);
}
return matching_locks;
***************
*** 1296,1302 **** static Query *
ApplyRetrieveRule(Query *parsetree,
RewriteRule *rule,
int rt_index,
- bool relation_level,
Relation relation,
List *activeRIRs,
bool forUpdatePushedDown)
--- 1289,1294 ----
***************
*** 1310,1317 **** ApplyRetrieveRule(Query *parsetree,
elog(ERROR, "expected just one rule action");
if (rule->qual != NULL)
elog(ERROR, "cannot handle qualified ON SELECT rule");
- if (!relation_level)
- elog(ERROR, "cannot handle per-attribute ON SELECT rule");
if (rt_index == parsetree->resultRelation)
{
--- 1302,1307 ----
***************
*** 1633,1646 **** fireRIRrules(Query *parsetree, List *activeRIRs, bool forUpdatePushedDown)
if (rule->event != CMD_SELECT)
continue;
- if (rule->attrno > 0)
- {
- /* per-attr rule; do we need it? */
- if (!attribute_used((Node *) parsetree, rt_index,
- rule->attrno, 0))
- continue;
- }
-
locks = lappend(locks, rule);
}
--- 1623,1628 ----
***************
*** 1665,1671 **** fireRIRrules(Query *parsetree, List *activeRIRs, bool forUpdatePushedDown)
parsetree = ApplyRetrieveRule(parsetree,
rule,
rt_index,
- rule->attrno == -1,
rel,
activeRIRs,
forUpdatePushedDown);
--- 1647,1652 ----
*** a/src/backend/rewrite/rewriteManip.c
--- b/src/backend/rewrite/rewriteManip.c
***************
*** 858,927 **** rangeTableEntry_used(Node *node, int rt_index, int sublevels_up)
/*
- * attribute_used -
- * Check if a specific attribute number of a RTE is used
- * somewhere in the query or expression.
- */
-
- typedef struct
- {
- int rt_index;
- int attno;
- int sublevels_up;
- } attribute_used_context;
-
- static bool
- attribute_used_walker(Node *node,
- attribute_used_context *context)
- {
- if (node == NULL)
- return false;
- if (IsA(node, Var))
- {
- Var *var = (Var *) node;
-
- if (var->varlevelsup == context->sublevels_up &&
- var->varno == context->rt_index &&
- var->varattno == context->attno)
- return true;
- return false;
- }
- if (IsA(node, Query))
- {
- /* Recurse into subselects */
- bool result;
-
- context->sublevels_up++;
- result = query_tree_walker((Query *) node, attribute_used_walker,
- (void *) context, 0);
- context->sublevels_up--;
- return result;
- }
- return expression_tree_walker(node, attribute_used_walker,
- (void *) context);
- }
-
- bool
- attribute_used(Node *node, int rt_index, int attno, int sublevels_up)
- {
- attribute_used_context context;
-
- context.rt_index = rt_index;
- context.attno = attno;
- context.sublevels_up = sublevels_up;
-
- /*
- * Must be prepared to start with a Query or a bare expression tree; if
- * it's a Query, we don't want to increment sublevels_up.
- */
- return query_or_expression_tree_walker(node,
- attribute_used_walker,
- (void *) &context,
- 0);
- }
-
-
- /*
* If the given Query is an INSERT ... SELECT construct, extract and
* return the sub-Query node that represents the SELECT part. Otherwise
* return the given Query.
--- 858,863 ----
*** a/src/backend/utils/adt/ruleutils.c
--- b/src/backend/utils/adt/ruleutils.c
***************
*** 3718,3724 **** make_ruledef(StringInfo buf, HeapTuple ruletup, TupleDesc rulettc,
char *rulename;
char ev_type;
Oid ev_class;
- int16 ev_attr;
bool is_instead;
char *ev_qual;
char *ev_action;
--- 3718,3723 ----
***************
*** 3745,3755 **** make_ruledef(StringInfo buf, HeapTuple ruletup, TupleDesc rulettc,
Assert(!isnull);
ev_class = DatumGetObjectId(dat);
- fno = SPI_fnumber(rulettc, "ev_attr");
- dat = SPI_getbinval(ruletup, rulettc, fno, &isnull);
- Assert(!isnull);
- ev_attr = DatumGetInt16(dat);
-
fno = SPI_fnumber(rulettc, "is_instead");
dat = SPI_getbinval(ruletup, rulettc, fno, &isnull);
Assert(!isnull);
--- 3744,3749 ----
***************
*** 3804,3813 **** make_ruledef(StringInfo buf, HeapTuple ruletup, TupleDesc rulettc,
/* The relation the rule is fired on */
appendStringInfo(buf, " TO %s", generate_relation_name(ev_class, NIL));
- if (ev_attr > 0)
- appendStringInfo(buf, ".%s",
- quote_identifier(get_relid_attribute_name(ev_class,
- ev_attr)));
/* If the rule has an event qualification, add it */
if (ev_qual == NULL)
--- 3798,3803 ----
***************
*** 3909,3915 **** make_viewdef(StringInfo buf, HeapTuple ruletup, TupleDesc rulettc,
Query *query;
char ev_type;
Oid ev_class;
- int16 ev_attr;
bool is_instead;
char *ev_qual;
char *ev_action;
--- 3899,3904 ----
***************
*** 3927,3935 **** make_viewdef(StringInfo buf, HeapTuple ruletup, TupleDesc rulettc,
fno = SPI_fnumber(rulettc, "ev_class");
ev_class = (Oid) SPI_getbinval(ruletup, rulettc, fno, &isnull);
- fno = SPI_fnumber(rulettc, "ev_attr");
- ev_attr = (int16) SPI_getbinval(ruletup, rulettc, fno, &isnull);
-
fno = SPI_fnumber(rulettc, "is_instead");
is_instead = (bool) SPI_getbinval(ruletup, rulettc, fno, &isnull);
--- 3916,3921 ----
***************
*** 3949,3955 **** make_viewdef(StringInfo buf, HeapTuple ruletup, TupleDesc rulettc,
query = (Query *) linitial(actions);
! if (ev_type != '1' || ev_attr >= 0 || !is_instead ||
strcmp(ev_qual, "<>") != 0 || query->commandType != CMD_SELECT)
{
appendStringInfo(buf, "Not a view");
--- 3935,3941 ----
query = (Query *) linitial(actions);
! if (ev_type != '1' || !is_instead ||
strcmp(ev_qual, "<>") != 0 || query->commandType != CMD_SELECT)
{
appendStringInfo(buf, "Not a view");
*** a/src/backend/utils/cache/relcache.c
--- b/src/backend/utils/cache/relcache.c
***************
*** 680,686 **** RelationBuildRuleLock(Relation relation)
rule->ruleId = HeapTupleGetOid(rewrite_tuple);
rule->event = rewrite_form->ev_type - '0';
- rule->attrno = rewrite_form->ev_attr;
rule->enabled = rewrite_form->ev_enabled;
rule->isInstead = rewrite_form->is_instead;
--- 680,685 ----
***************
*** 796,803 **** equalRuleLocks(RuleLock *rlock1, RuleLock *rlock2)
return false;
if (rule1->event != rule2->event)
return false;
- if (rule1->attrno != rule2->attrno)
- return false;
if (rule1->enabled != rule2->enabled)
return false;
if (rule1->isInstead != rule2->isInstead)
--- 795,800 ----
*** a/src/include/rewrite/prs2lock.h
--- b/src/include/rewrite/prs2lock.h
***************
*** 25,31 **** typedef struct RewriteRule
{
Oid ruleId;
CmdType event;
- AttrNumber attrno;
Node *qual;
List *actions;
char enabled;
--- 25,30 ----
*** a/src/include/rewrite/rewriteManip.h
--- b/src/include/rewrite/rewriteManip.h
***************
*** 49,56 **** extern void IncrementVarSublevelsUp_rtable(List *rtable,
extern bool rangeTableEntry_used(Node *node, int rt_index,
int sublevels_up);
! extern bool attribute_used(Node *node, int rt_index, int attno,
! int sublevels_up);
extern Query *getInsertSelectQuery(Query *parsetree, Query ***subquery_ptr);
--- 49,55 ----
extern bool rangeTableEntry_used(Node *node, int rt_index,
int sublevels_up);
! extern bool attribute_used(Node *node, int rt_index, int sublevels_up);
extern Query *getInsertSelectQuery(Query *parsetree, Query ***subquery_ptr);
*** a/src/tools/pgindent/typedefs.list
--- b/src/tools/pgindent/typedefs.list
***************
*** 1955,1961 **** adjust_appendrel_attrs_context
allocfunc
array_unnest_fctx
assign_collations_context
- attribute_used_context
autovac_table
av_relation
avl_dbase
--- 1955,1960 ----
*** a/doc/src/sgml/catalogs.sgml
--- b/doc/src/sgml/catalogs.sgml
***************
*** 5024,5030 ****
<entry><structfield>ev_attr</structfield></entry>
<entry><type>int2</type></entry>
<entry></entry>
! <entry>The column this rule is for (currently, always -1 to
indicate the whole table)</entry>
</row>
--- 5024,5030 ----
<entry><structfield>ev_attr</structfield></entry>
<entry><type>int2</type></entry>
<entry></entry>
! <entry>The column this rule is for (currently, always zero to
indicate the whole table)</entry>
</row>
*** a/src/backend/rewrite/rewriteDefine.c
--- b/src/backend/rewrite/rewriteDefine.c
***************
*** 489,495 **** DefineQueryRewrite(char *rulename,
/*
* This rule is allowed - prepare to install it.
*/
! event_attno = -1;
/* discard rule if it's null action and not INSTEAD; it's a no-op */
if (action != NIL || is_instead)
--- 489,495 ----
/*
* This rule is allowed - prepare to install it.
*/
! event_attno = 0;
/* discard rule if it's null action and not INSTEAD; it's a no-op */
if (action != NIL || is_instead)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
