Re: system catalog pg_rewrite column ev_attr document description problem

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Kevin Grittner <kgrittn(at)ymail(dot)com> writes:
>> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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