Re: Doc patch to note which system catalogs have oids

Hi,

The attached patch documents the oid column of those
system catalogs having an oid.

Distinguish system catalogs with an oid from those without
and make the primary key clear to the newbie.

Found catalogs with an oid by querying a 9.2 installation:

select pg_class.relkind, pg_class.relname
from pg_class, pg_attribute
where pg_attribute.attrelid = pg_class.oid
and pg_attribute.attname = 'oid'
and pg_class.relname like 'pg_%'
and (pg_class.relkind = 'r' -- table
or pg_class.relkind = 'v') -- view
order by pg_class.relkind, pg_class.relname;

Karl <kop(at)meme(dot)com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein

"Karl O. Pinc" <kop(at)meme(dot)com> writes:
> The attached patch documents the oid column of those
> system catalogs having an oid.

I think this is fundamentally wrong, or at least misleading, because it
documents OID as if it were an ordinary column. Somebody who did
"select * from pg_class" and didn't see any "oid" in the result would
think the docs were wrong.

It's possible that it's worth expending a boilerplate paragraph in each
of those pages to say "this catalog has OIDs" (or that it doesn't).
But this isn't the way.

regards, tom lane

On 09/23/2012 10:14:33 PM, Tom Lane wrote:
> "Karl O. Pinc" <kop(at)meme(dot)com> writes:
> > The attached patch documents the oid column of those
> > system catalogs having an oid.
>
> I think this is fundamentally wrong, or at least misleading, because
> it
> documents OID as if it were an ordinary column. Somebody who did
> "select * from pg_class" and didn't see any "oid" in the result would
> think the docs were wrong.

Ok.

When I went looking at querying the
system catalogs I got confused some time ago because oids were
not listed along with the other columns. (It didn't help that the
catalog I was looking at had another column of type oid.)

>
> It's possible that it's worth expending a boilerplate paragraph in
> each
> of those pages to say "this catalog has OIDs" (or that it doesn't).
> But this isn't the way.

How about modifying the ("printed") table layout as attached?
It begins each ("printed") table documenting each catalog with a
"Has OID column" Yes/No.

Also, I note that pg_constraint and pg_collation are not
collated properly in the docs. (Constraint comes before collation
in the docs, although everything else is sorted by name.)

A second patch (applied on top of the first) fixes this.

Karl <kop(at)meme(dot)com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein

Tom,

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> I think this is fundamentally wrong, or at least misleading, because it
> documents OID as if it were an ordinary column. Somebody who did
> "select * from pg_class" and didn't see any "oid" in the result would
> think the docs were wrong.

Given that it's been quite some time since we defaulted to including
OIDs in tables, and the high level of confusion that individuals trying
to join pg_class and pg_namespace together go through due to select *
not including the oid column, I wonder if perhaps we should consider
changing that. Might be possible to do for just the catalog tables (to
minimize the risk of breaking poorly-written applications), or provide
a GUC to control including the oid column in select *.

> It's possible that it's worth expending a boilerplate paragraph in each
> of those pages to say "this catalog has OIDs" (or that it doesn't).
> But this isn't the way.

I'm afraid I disagree with this. The oid column, in the system
catalog, is user-facing and I like having it included as a column in the
table in the docs, so users know what to use when doing joins.
Including something in the boilerplate about it not being shown by
default (or in the description in the table) might be alright, if we
don't change that.

Thanks,

Stephen

On 09/24/2012 09:38:53 AM, Karl O. Pinc wrote:
> On 09/23/2012 10:14:33 PM, Tom Lane wrote:
> > "Karl O. Pinc" <kop(at)meme(dot)com> writes:
> > > The attached patch documents the oid column of those
> > > system catalogs having an oid.
> >
> > I think this is fundamentally wrong, or at least misleading,
> because
> > it
> > documents OID as if it were an ordinary column.

> How about modifying the ("printed") table layout as attached?
> It begins each ("printed") table documenting each catalog with a
> "Has OID column" Yes/No.

Changed text from "Has OID column" to "Keyed with an OID column"
since this explains more and there's no worry about horizontal
space.

I like having the documentation of oid be part of the
(printed) table describing the columns, in some way or
another, since that's where the eye is drawn when
looking for column documentation.

Karl <kop(at)meme(dot)com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein

On 09/24/2012 08:18:00 PM, Stephen Frost wrote:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:

> > It's possible that it's worth expending a boilerplate paragraph in
> each
> > of those pages to say "this catalog has OIDs" (or that it doesn't).
> > But this isn't the way.
>
> I'm afraid I disagree with this. The oid column, in the system
> catalog, is user-facing and I like having it included as a column in
> the
> table in the docs, so users know what to use when doing joins.
> Including something in the boilerplate about it not being shown by
> default (or in the description in the table) might be alright, if we
> don't change that.

Having information about oid included in the (printed) table
under a separate heading, as in v2 and v3 of this patch,
is something of a compromise. It's hard to visualize
from the sgml so it might be worth building the docs
and viewing with a file:/// url. The trouble is that
it's visually ugly because the two parts of the
table are of separate widths. There is almost surely a way
to change this in the xsl transformation to html/etc., but I
would probably do a bad job of it and can't
speak to the sanity of maintaining such a thing.
(So it's probably a bad idea.)

Karl <kop(at)meme(dot)com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein

On 09/23/2012 08:57:45 PM, Karl O. Pinc wrote:

> The attached patch documents the oid column of those
> system catalogs having an oid.

Don't use the first version of this patch (oid_doc.patch)
without discarding the last hunk. The last hunk
introduces an error by duplicating the
documentation of the pg_roles.oid column.

(I am reluctant to post a revised version
since there's already 3 versions floating
around representing 2 different approaches
and no consensus as to which approach, if any, should
be taken.)

Regards,

Karl <kop(at)meme(dot)com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein

On Mon, Sep 24, 2012 at 9:18 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> I think this is fundamentally wrong, or at least misleading, because it
>> documents OID as if it were an ordinary column. Somebody who did
>> "select * from pg_class" and didn't see any "oid" in the result would
>> think the docs were wrong.
>
> Given that it's been quite some time since we defaulted to including
> OIDs in tables, and the high level of confusion that individuals trying
> to join pg_class and pg_namespace together go through due to select *
> not including the oid column, I wonder if perhaps we should consider
> changing that. Might be possible to do for just the catalog tables (to
> minimize the risk of breaking poorly-written applications), or provide
> a GUC to control including the oid column in select *.
>
>> It's possible that it's worth expending a boilerplate paragraph in each
>> of those pages to say "this catalog has OIDs" (or that it doesn't).
>> But this isn't the way.
>
> I'm afraid I disagree with this. The oid column, in the system
> catalog, is user-facing and I like having it included as a column in the
> table in the docs, so users know what to use when doing joins.

+1.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 09/25/2012 12:28:13 AM, Karl O. Pinc wrote:
> On 09/23/2012 08:57:45 PM, Karl O. Pinc wrote:
>
> > The attached patch documents the oid column of those
> > system catalogs having an oid.
>
> Don't use the first version of this patch (oid_doc.patch)
> without discarding the last hunk. The last hunk
> introduces an error by duplicating the
> documentation of the pg_roles.oid column.
>
> (I am reluctant to post a revised version
> since there's already 3 versions floating
> around representing 2 different approaches
> and no consensus as to which approach, if any, should
> be taken.)

I have figured out how to use the commitfest
pages to track what should be reviewed.
Submitting a corrected version of the very
first patch, which treats the oids as
regular columns.

I am now submitting patches to the commitfest
for review. (I'm not sure how I missed this.)

Regards,

Karl <kop(at)meme(dot)com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein

On Tue, 2012-10-02 at 10:46 -0500, Karl O. Pinc wrote:
> I am now submitting patches to the commitfest
> for review. (I'm not sure how I missed this.)

I prefer this version of the patch. I also attached an alternative
version that may address Tom's concern by noting that the OIDs are
hidden in the description.

Marking "ready for committer".

Regards,
Jeff Davis

On 12/14/2012 02:04:45 AM, Jeff Davis wrote:
> On Tue, 2012-10-02 at 10:46 -0500, Karl O. Pinc wrote:
> > I am now submitting patches to the commitfest
> > for review. (I'm not sure how I missed this.)
>
> I prefer this version of the patch. I also attached an alternative
> version that may address Tom's concern by noting that the OIDs are
> hidden in the description.

For the record, the preferred version referred to above is:

oid_doc_v4.patch

> Marking "ready for committer".

Thanks!

Karl <kop(at)meme(dot)com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein

On Fri, 2012-12-14 at 00:04 -0800, Jeff Davis wrote:
> On Tue, 2012-10-02 at 10:46 -0500, Karl O. Pinc wrote:
> > I am now submitting patches to the commitfest
> > for review. (I'm not sure how I missed this.)
>
> I prefer this version of the patch. I also attached an alternative
> version that may address Tom's concern by noting that the OIDs are
> hidden in the description.

Committed this version.