Cleanup/remove/update references to OID column

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

Cleanup/remove/update references to OID column

Justin Pryzby
Cleanup/remove/update references to OID column...

..in wake of 578b229718e8f.

See also
93507e67c9ca54026019ebec3026de35d30370f9
1464755fc490a9911214817fe83077a3689250ab
---
 doc/src/sgml/ddl.sgml          |  9 ++++-----
 doc/src/sgml/ref/insert.sgml   | 12 +++++-------
 doc/src/sgml/ref/psql-ref.sgml |  3 +++
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 9e761db..db044c5 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -3672,11 +3672,10 @@ VALUES ('Albany', NULL, NULL, 'NY');
       <para>
        Partitions cannot have columns that are not present in the parent.  It
        is not possible to specify columns when creating partitions with
-       <command>CREATE TABLE</command>, nor is it possible to add columns to
-       partitions after-the-fact using <command>ALTER TABLE</command>.  Tables may be
-       added as a partition with <command>ALTER TABLE ... ATTACH PARTITION</command>
-       only if their columns exactly match the parent, including any
-       <literal>oid</literal> column.
+       <command>CREATE TABLE</command>, to add columns to
+       partitions after-the-fact using <command>ALTER TABLE</command>, nor to
+       add a partition with <command>ALTER TABLE ... ATTACH PARTITION</command>
+       if its columns would not exactly match those of the parent.
       </para>
      </listitem>
 
diff --git a/doc/src/sgml/ref/insert.sgml b/doc/src/sgml/ref/insert.sgml
index 62e142f..3e1be4c 100644
--- a/doc/src/sgml/ref/insert.sgml
+++ b/doc/src/sgml/ref/insert.sgml
@@ -552,13 +552,11 @@ INSERT INTO <replaceable class="parameter">table_name</replaceable> [ AS <replac
 INSERT <replaceable>oid</replaceable> <replaceable class="parameter">count</replaceable>
 </screen>
    The <replaceable class="parameter">count</replaceable> is the
-   number of rows inserted or updated.  If <replaceable
-   class="parameter">count</replaceable> is exactly one, and the
-   target table has OIDs, then <replaceable
-   class="parameter">oid</replaceable> is the <acronym>OID</acronym>
-   assigned to the inserted row.  The single row must have been
-   inserted rather than updated.  Otherwise <replaceable
-   class="parameter">oid</replaceable> is zero.
+   number of rows inserted or updated.
+   <replaceable>oid</replaceable> used to be the object ID of the inserted row
+   if <replaceable>rows</replaceable> was 1 and the target table had OIDs, but
+   OIDs system columns are not supported anymore; therefore
+   <replaceable>oid</replaceable> is always 0.
   </para>
 
   <para>
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 08f4bab..0e6e792 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -3794,6 +3794,9 @@ bar
         command. This variable is only guaranteed to be valid until
         after the result of the next <acronym>SQL</acronym> command has
         been displayed.
+        <productname>PostgreSQL</productname> servers since version 12 do not
+        support OID system columns in user tables, and LASTOID will always be 0
+        following <command>INSERT</command>.
         </para>
         </listitem>
       </varlistentry>
--
2.1.4


v1-0001-Cleanup-remove-update-references-to-OID-column.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Cleanup/remove/update references to OID column

Daniel Verite
        Justin Pryzby wrote:

> Cleanup/remove/update references to OID column...
>
> ..in wake of 578b229718e8f.

Just spotted a couple of other references that need updates:

#1. In catalogs.sgml:

     <row>
      <entry><structfield>attnum</structfield></entry>
      <entry><type>int2</type></entry>
      <entry></entry>
      <entry>
       The number of the column.  Ordinary columns are numbered from 1
       up.  System columns, such as <structfield>oid</structfield>,
       have (arbitrary) negative numbers.
      </entry>
     </row>

oid should be replaced by xmin or some other system column.


#2. In ddl.sgml, when describing ctid:

     <para>
      The physical location of the row version within its table.  Note that
      although the <structfield>ctid</structfield> can be used to
      locate the row version very quickly, a row's
      <structfield>ctid</structfield> will change if it is
      updated or moved by <command>VACUUM FULL</command>.  Therefore
      <structfield>ctid</structfield> is useless as a long-term row
      identifier.  The OID, or even better a user-defined serial
      number, should be used to identify logical rows.
     </para>

"The OID" used to refer to an entry above in that list, now it's not
clear what it refers to.
"serial number" also sounds somewhat obsolete now that IDENTITY is
supported. The last sentence could be changed to:
 "A primary key should be used to identify logical rows".


Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


Reply | Threaded
Open this post in threaded view
|

Re: Cleanup/remove/update references to OID column

Justin Pryzby
On Wed, Apr 10, 2019 at 06:32:35PM +0200, Daniel Verite wrote:
> Justin Pryzby wrote:
>
> > Cleanup/remove/update references to OID column...
>
> Just spotted a couple of other references that need updates:

> #1. In catalogs.sgml:
> #2. In ddl.sgml, when describing ctid:

I found and included fixes for a few more references:

 doc/src/sgml/catalogs.sgml           | 2 +-
 doc/src/sgml/ddl.sgml                | 3 +--
 doc/src/sgml/information_schema.sgml | 4 ++--
 doc/src/sgml/ref/create_trigger.sgml | 2 +-
 doc/src/sgml/spi.sgml                | 2 +-

Justin

v2-0001-Cleanup-remove-update-references-to-OID-column.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Cleanup/remove/update references to OID column

Michael Paquier-2
On Wed, Apr 10, 2019 at 11:59:18AM -0500, Justin Pryzby wrote:
> I found and included fixes for a few more references:
>
>  doc/src/sgml/catalogs.sgml           | 2 +-
>  doc/src/sgml/ddl.sgml                | 3 +--
>  doc/src/sgml/information_schema.sgml | 4 ++--
>  doc/src/sgml/ref/create_trigger.sgml | 2 +-
>  doc/src/sgml/spi.sgml                | 2 +-

Open Item++.

Andres, as this is a consequence of 578b229, could you look at what is
proposed here?
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Cleanup/remove/update references to OID column

Andres Freund
Hi,

On 2019-04-11 13:26:38 +0900, Michael Paquier wrote:

> On Wed, Apr 10, 2019 at 11:59:18AM -0500, Justin Pryzby wrote:
> > I found and included fixes for a few more references:
> >
> >  doc/src/sgml/catalogs.sgml           | 2 +-
> >  doc/src/sgml/ddl.sgml                | 3 +--
> >  doc/src/sgml/information_schema.sgml | 4 ++--
> >  doc/src/sgml/ref/create_trigger.sgml | 2 +-
> >  doc/src/sgml/spi.sgml                | 2 +-
>
> Open Item++.

> Andres, as this is a consequence of 578b229, could you look at what is
> proposed here?

Yes, I was planning to commit that soon-ish. There still seemed
review / newer versions happening, though, so I was thinking of waiting
for a bit longer.

- Andres


Reply | Threaded
Open this post in threaded view
|

Re: Cleanup/remove/update references to OID column

Justin Pryzby
On Thu, Apr 11, 2019 at 08:39:42AM -0700, Andres Freund wrote:
> Yes, I was planning to commit that soon-ish. There still seemed
> review / newer versions happening, though, so I was thinking of waiting
> for a bit longer.

I don't expect anything new.

I got all that from: git grep '>oid' doc/src/sgml, so perhaps you'd want to
check for any other stragglers.

Thanks,
Justin


Reply | Threaded
Open this post in threaded view
|

Re: Cleanup/remove/update references to OID column

Daniel Verite
In reply to this post by Andres Freund
        Andres Freund wrote:

> Yes, I was planning to commit that soon-ish. There still seemed
> review / newer versions happening, though, so I was thinking of waiting
> for a bit longer.

You might want to apply this trivial one in the same batch:

index 452f307..7cfb67f 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -428,7 +428,7 @@ main(int argc, char **argv)
 
        InitDumpOptions(&dopt);
 
- while ((c = getopt_long(argc, argv,
"abBcCd:E:f:F:h:j:n:N:oOp:RsS:t:T:U:vwWxZ:",
+ while ((c = getopt_long(argc, argv,
"abBcCd:E:f:F:h:j:n:N:Op:RsS:t:T:U:vwWxZ:",
                                                        long_options,
&optindex)) != -1)
        {
                switch (c)

"o" in the options list is a leftover. Leaving it in getopt_long() has the
effect that pg_dump -o fails (per the default case in the switch),
but it's missing the expected error message (pg_dump: invalid option -- 'o')


Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


Reply | Threaded
Open this post in threaded view
|

Re: Cleanup/remove/update references to OID column

Andres Freund
In reply to this post by Justin Pryzby
Hi,

On 2019-04-10 11:59:18 -0500, Justin Pryzby wrote:

> @@ -1202,8 +1202,7 @@ CREATE TABLE circles (
>        <structfield>ctid</structfield> will change if it is
>        updated or moved by <command>VACUUM FULL</command>.  Therefore
>        <structfield>ctid</structfield> is useless as a long-term row
> -      identifier.  The OID, or even better a user-defined serial
> -      number, should be used to identify logical rows.
> +      identifier.  A primary key should be used to identify logical rows.
>       </para>
>      </listitem>
>     </varlistentry>

That works for me.


> @@ -3672,11 +3671,10 @@ VALUES ('Albany', NULL, NULL, 'NY');
>        <para>
>         Partitions cannot have columns that are not present in the parent.  It
>         is not possible to specify columns when creating partitions with
> -       <command>CREATE TABLE</command>, nor is it possible to add columns to
> -       partitions after-the-fact using <command>ALTER TABLE</command>.  Tables may be
> -       added as a partition with <command>ALTER TABLE ... ATTACH PARTITION</command>
> -       only if their columns exactly match the parent, including any
> -       <literal>oid</literal> column.
> +       <command>CREATE TABLE</command>, to add columns to
> +       partitions after-the-fact using <command>ALTER TABLE</command>, nor to
> +       add a partition with <command>ALTER TABLE ... ATTACH PARTITION</command>
> +       if its columns would not exactly match those of the parent.
>        </para>
>       </listitem>

This seems like a bigger change than necessary. I just chopped off the
"including any oid column".



> diff --git a/doc/src/sgml/ref/create_trigger.sgml b/doc/src/sgml/ref/create_trigger.sgml
> index 6456105..3339a4b 100644
> --- a/doc/src/sgml/ref/create_trigger.sgml
> +++ b/doc/src/sgml/ref/create_trigger.sgml
> @@ -465,7 +465,7 @@ UPDATE OF <replaceable>column_name1</replaceable> [, <replaceable>column_name2</
>     that the <literal>NEW</literal> row seen by the condition is the current value,
>     as possibly modified by earlier triggers.  Also, a <literal>BEFORE</literal>
>     trigger's <literal>WHEN</literal> condition is not allowed to examine the
> -   system columns of the <literal>NEW</literal> row (such as <literal>oid</literal>),
> +   system columns of the <literal>NEW</literal> row (such as <literal>ctid</literal>),
>     because those won't have been set yet.
>    </para>

Hm. Not because of your change, but this sentence seems wrong. For one,
"is not allowed" isn't really true - one can very well write a trigger
doing so. The returned values just are bogus.

CREATE OR REPLACE FUNCTION scream_sysattrs() RETURNS TRIGGER LANGUAGE
plpgsql AS $$
BEGIN
    RAISE NOTICE 'inserted row: self: % xmin: % cmin: %, xmax: %, cmax: % tableoid: %', NEW.ctid, NEW.xmin, NEW.cmin, NEW.xmax, NEW.cmax, NEW.tableoid;
    RETURN NEW;
END;$$;
DROP TABLE IF EXISTS foo; CREATE TABLE foo(i int);CREATE TRIGGER foo BEFORE INSERT ON foo FOR EACH ROW EXECUTE FUNCTION scream_sysattrs();
postgres[5532][1]=# INSERT INTO foo values(1);
NOTICE:  00000: inserted row: self: (0,0) xmin: 112 cmin: 2249, xmax: 4294967295, cmax: 2249 tableoid: 0
LOCATION:  exec_stmt_raise, pl_exec.c:3778
INSERT 0 1

We probably should do better...



> diff --git a/doc/src/sgml/ref/insert.sgml b/doc/src/sgml/ref/insert.sgml
> index 62e142f..3e1be4c 100644
> --- a/doc/src/sgml/ref/insert.sgml
> +++ b/doc/src/sgml/ref/insert.sgml
> @@ -552,13 +552,11 @@ INSERT INTO <replaceable class="parameter">table_name</replaceable> [ AS <replac
>  INSERT <replaceable>oid</replaceable> <replaceable class="parameter">count</replaceable>
>  </screen>
>     The <replaceable class="parameter">count</replaceable> is the
> -   number of rows inserted or updated.  If <replaceable
> -   class="parameter">count</replaceable> is exactly one, and the
> -   target table has OIDs, then <replaceable
> -   class="parameter">oid</replaceable> is the <acronym>OID</acronym>
> -   assigned to the inserted row.  The single row must have been
> -   inserted rather than updated.  Otherwise <replaceable
> -   class="parameter">oid</replaceable> is zero.
> +   number of rows inserted or updated.
> +   <replaceable>oid</replaceable> used to be the object ID of the inserted row
> +   if <replaceable>rows</replaceable> was 1 and the target table had OIDs, but
> +   OIDs system columns are not supported anymore; therefore
> +   <replaceable>oid</replaceable> is always 0.
>    </para>

I rephrased this a bit. Felt like the important bit came after
historical information:
+   The <replaceable class="parameter">count</replaceable> is the number of
+   rows inserted or updated.  <replaceable>oid</replaceable> is always 0 (it
+   used to be the <acronym>OID</acronym> assigned to the inserted row if
+   <replaceable>rows</replaceable> was exactly one and the target table was
+   declared <literal>WITH OIDS</literal> and 0 otherwise, but creating a table
+   <literal>WITH OIDS</literal> is not supported anymore).


>    <para>
> diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
> index 08f4bab..0e6e792 100644
> --- a/doc/src/sgml/ref/psql-ref.sgml
> +++ b/doc/src/sgml/ref/psql-ref.sgml
> @@ -3794,6 +3794,9 @@ bar
>          command. This variable is only guaranteed to be valid until
>          after the result of the next <acronym>SQL</acronym> command has
>          been displayed.
> +        <productname>PostgreSQL</productname> servers since version 12 do not
> +        support OID system columns in user tables, and LASTOID will always be 0
> +        following <command>INSERT</command>.
>          </para>
>          </listitem>
>        </varlistentry>

It's not just user tables, system tables as well (it's just an ordinary
table now). I also though it might be good to clarify that LASTOID still
works for older servers.

+        <productname>PostgreSQL</productname> servers since version 12 do not
+        support OID system columns anymore, thus LASTOID will always be 0
+        following <command>INSERT</command> when targeting such servers.


Thanks for the patch!

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: Cleanup/remove/update references to OID column

Andres Freund
In reply to this post by Daniel Verite
Hi,

On 2019-04-15 18:35:12 +0200, Daniel Verite wrote:

> Andres Freund wrote:
>
> > Yes, I was planning to commit that soon-ish. There still seemed
> > review / newer versions happening, though, so I was thinking of waiting
> > for a bit longer.
>
> You might want to apply this trivial one in the same batch:
>
> index 452f307..7cfb67f 100644
> --- a/src/bin/pg_dump/pg_dump.c
> +++ b/src/bin/pg_dump/pg_dump.c
> @@ -428,7 +428,7 @@ main(int argc, char **argv)
>  
> InitDumpOptions(&dopt);
>  
> - while ((c = getopt_long(argc, argv,
> "abBcCd:E:f:F:h:j:n:N:oOp:RsS:t:T:U:vwWxZ:",
> + while ((c = getopt_long(argc, argv,
> "abBcCd:E:f:F:h:j:n:N:Op:RsS:t:T:U:vwWxZ:",
> long_options,
> &optindex)) != -1)
> {
> switch (c)
>
> "o" in the options list is a leftover. Leaving it in getopt_long() has the
> effect that pg_dump -o fails (per the default case in the switch),
> but it's missing the expected error message (pg_dump: invalid option -- 'o')

Thanks for finding! Pushed.

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: Cleanup/remove/update references to OID column

Justin Pryzby
In reply to this post by Andres Freund
On Wed, Apr 17, 2019 at 05:23:47PM -0700, Andres Freund wrote:
> Thanks for the patch!

Thanks for fixing it up and commiting.

Would you consider the remaining two hunks, attached ?

Justin

v3-0001-Cleanup-remove-update-references-to-OID-column.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Cleanup/remove/update references to OID column

Andres Freund
Hi,

On 2019-04-17 19:42:19 -0500, Justin Pryzby wrote:

> diff --git a/doc/src/sgml/information_schema.sgml b/doc/src/sgml/information_schema.sgml
> index 234a3bb..9c618b1 100644
> --- a/doc/src/sgml/information_schema.sgml
> +++ b/doc/src/sgml/information_schema.sgml
> @@ -1312,8 +1312,8 @@
>    <para>
>     The view <literal>columns</literal> contains information about all
>     table columns (or view columns) in the database.  System columns
> -   (<literal>ctid</literal>, etc.) are not included.  Only those columns are
> -   shown that the current user has access to (by way of being the
> +   (<literal>ctid</literal>, etc.) are not included.  The only columns shown
> +   are those to which the current user has access (by way of being the
>     owner or having some privilege).
>    </para>

I don't see the point of this change? Nor what it has to do with oids?


> diff --git a/doc/src/sgml/ref/insert.sgml b/doc/src/sgml/ref/insert.sgml
> index 189ce2a..f995a76 100644
> --- a/doc/src/sgml/ref/insert.sgml
> +++ b/doc/src/sgml/ref/insert.sgml
> @@ -554,7 +554,7 @@ INSERT <replaceable>oid</replaceable> <replaceable class="parameter">count</repl
>     The <replaceable class="parameter">count</replaceable> is the number of
>     rows inserted or updated.  <replaceable>oid</replaceable> is always 0 (it
>     used to be the <acronym>OID</acronym> assigned to the inserted row if
> -   <replaceable>rows</replaceable> was exactly one and the target table was
> +   <replaceable>count</replaceable> was exactly one and the target table was
>     declared <literal>WITH OIDS</literal> and 0 otherwise, but creating a table
>     <literal>WITH OIDS</literal> is not supported anymore).
>    </para>

The <replacable>rows</<replacable> reference is from your change
:(. I'll fold it into another upcoming change for other tableam comment
improvements (by Heikki).

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: Cleanup/remove/update references to OID column

Justin Pryzby
On Wed, Apr 17, 2019 at 05:51:15PM -0700, Andres Freund wrote:

> Hi,
>
> On 2019-04-17 19:42:19 -0500, Justin Pryzby wrote:
> > diff --git a/doc/src/sgml/information_schema.sgml b/doc/src/sgml/information_schema.sgml
> > index 234a3bb..9c618b1 100644
> > --- a/doc/src/sgml/information_schema.sgml
> > +++ b/doc/src/sgml/information_schema.sgml
> > @@ -1312,8 +1312,8 @@
> >    <para>
> >     The view <literal>columns</literal> contains information about all
> >     table columns (or view columns) in the database.  System columns
> > -   (<literal>ctid</literal>, etc.) are not included.  Only those columns are
> > -   shown that the current user has access to (by way of being the
> > +   (<literal>ctid</literal>, etc.) are not included.  The only columns shown
> > +   are those to which the current user has access (by way of being the
> >     owner or having some privilege).
> >    </para>
>
> I don't see the point of this change? Nor what it has to do with oids?

It doesn't have to do with oids, but seems more correct and cleaner...to my
eyes.

> > -   <replaceable>rows</replaceable> was exactly one and the target table was
> > +   <replaceable>count</replaceable> was exactly one and the target table was
> The <replacable>rows</<replacable> reference is from your change
> :(.

Ouch, not sure how I did that..sorry for the noise (twice).

Thanks,
Justin