system catalog pg_rewrite column ev_attr document description problem
Hi,
system catalog pg_rewrite column ev_attr document description as shown below
ev_attr - The column this rule is for (currently, always zero to indicate
the whole table)
But In the code the column value is always set as -1. can we change the
column description as below is fine?
ev_attr - The column this rule is for, presently this value is -1.
Regards,
Hari babu.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hari Babu <haribabu.kommi@huawei.com> wrote:
system catalog pg_rewrite column ev_attr document description as shown below
ev_attr - The column this rule is for (currently, always zero to indicate
the whole table)But In the code the column value is always set as -1. can we change the
column description as below is fine?ev_attr - The column this rule is for, presently this value is -1.
I just changed "zero" to "-1".
Thanks for the report!
--
Kevin Grittner
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Kevin Grittner <kgrittn@ymail.com> writes:
Hari Babu <haribabu.kommi@huawei.com> wrote:
system catalog pg_rewrite column ev_attr document description as shown below
ev_attr� - The column this rule is for (currently, always zero to indicate
the whole table)But In the code the column value is always set as -1. can we change the
column description as below is fine?ev_attr� - The column this rule is for, presently this value is -1.
I just changed "zero" to "-1".
Actually, I think this is a bug and the right thing is to make the code
match the documentation not vice versa. ev_attr isn't being used for
much at the moment, but if it were being used as an AttrNumber, -1 would
not mean "whole row". It would be a reference to the system column with
number -1 (ctid, if memory serves). Zero is the usual choice for a
whole-row reference.
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
Tom Lane <tgl@sss.pgh.pa.us> wrote:
Actually, I think this is a bug and the right thing is to make the code
match the documentation not vice versa. ev_attr isn't being used for
much at the moment, but if it were being used as an AttrNumber, -1 would
not mean "whole row". It would be a reference to the system column
with number -1 (ctid, if memory serves). Zero is the usual choice for a
whole-row reference.
I figured that since the docs for all supported production versions
give incorrect information, I should backpatch this, which I did
before seeing your post.
I assume that this should be a 9.3 code fix, and a doc fix prior to
that, since it would require changing catalogs and might break
existing user queries? Should the docs mention the value used in
each version, or be changed to just be silent on the issue?
Such a change would require a catversion bump.
Such a change would require mention in the release notes because
existing user queries against pg_rewrite might fail unless
adjusted.
Is it worth doing that now, versus when and if the hypothetical
change to reference a column is made?
-Kevin
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Kevin Grittner <kgrittn@ymail.com> writes:
Tom Lane <tgl@sss.pgh.pa.us> wrote:
Actually, I think this is a bug and the right thing is to make the code
match the documentation not vice versa.
I assume that this should be a 9.3 code fix, and a doc fix prior to
that, since it would require changing catalogs and might break
existing user queries?� Should the docs mention the value used in
each version, or be changed to just be silent on the issue?
I think the odds that any user queries are looking at that column are
pretty negligible, especially since nobody has complained about the
inaccurate documentation previously. I agree with only changing the
behavior in HEAD, just in case, but I don't see any strong reason to
jump through hoops here.
Such a change would require a catversion bump.
Not really. There appears to be one place in ruleutils.c that would
need to be tweaked to allow either -1 or 0 (the other place already
does, so the code is inconsistent now anyhow).
Such a change would require mention in the release notes because
existing user queries against pg_rewrite might fail unless
adjusted.
I would not bother with that either; seems like a waste of readers'
attention span.
Is it worth doing that now, versus when and if the hypothetical
change to reference a column is made?
Well, the longer we leave it as-is, the greater risk that somebody
might write code that really does depend on the bogus value.
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
Tom Lane <tgl@sss.pgh.pa.us> wrote:
Kevin Grittner <kgrittn@ymail.com> writes:
Tom Lane <tgl@sss.pgh.pa.us> wrote:
Actually, I think this is a bug and the right thing is to make the code
match the documentation not vice versa.I assume that this should be a 9.3 code fix, and a doc fix prior to
that, since it would require changing catalogs and might break
existing user queries? Should the docs mention the value used in
each version, or be changed to just be silent on the issue?I think the odds that any user queries are looking at that column are
pretty negligible, especially since nobody has complained about the
inaccurate documentation previously. I agree with only changing the
behavior in HEAD, just in case, but I don't see any strong reason to
jump through hoops here.Such a change would require a catversion bump.
Not really. There appears to be one place in ruleutils.c that would
need to be tweaked to allow either -1 or 0 (the other place already
does, so the code is inconsistent now anyhow).Such a change would require mention in the release notes because
existing user queries against pg_rewrite might fail unless
adjusted.I would not bother with that either; seems like a waste of readers'
attention span.Is it worth doing that now, versus when and if the hypothetical
change to reference a column is made?Well, the longer we leave it as-is, the greater risk that somebody
might write code that really does depend on the bogus value.
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.
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.
--
Kevin Grittner
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Kevin Grittner <kgrittn@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
Attachments:
rule-attr-part1.patchtext/x-diff; name=rule-attr-part1.patchDownload
*** 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 ----
rule-attr-part2.patchtext/x-diff; name=rule-attr-part2.patchDownload
*** 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)
Kevin Grittner <kgrittn@ymail.com> writes:
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.
If we're going to do that, there doesn't seem to me to be a lot of point
in adjusting the column's contents in 9.3 --- the argument for doing
that was mostly forward compatibility with some future actual usage of
the column. I'd be more inclined to leave HEAD as-is and then do the
column-ectomy along with this removal of dead code in 9.4.
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
Tom Lane <tgl@sss.pgh.pa.us> wrote:
Kevin Grittner <kgrittn@ymail.com> writes:
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.
If we're going to do that, there doesn't seem to me to be a lot of point
in adjusting the column's contents in 9.3 --- the argument for doing
that was mostly forward compatibility with some future actual usage of
the column. I'd be more inclined to leave HEAD as-is and then do the
column-ectomy along with this removal of dead code in 9.4.
ok
--
Kevin Grittner
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers