Re: Cleanup/remove/update references to OID column

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