Обсуждение: 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.

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

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

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

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

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

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

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

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