Cleanup/remove/update references to OID column

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

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

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

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

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

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

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

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

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

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

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

On Wed, Apr 17, 2019 at 11:14:13PM -0500, Justin Pryzby wrote:

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

-   <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).

For the record, I found that I borrowed the language from
578b229718e8f:doc/src/sgml/protocol.sgml (but should have borrowed a bit less).

Justin

I found what appears to be a dangling reference to old "hidden" OID behavior.

Justin

I'm resending this patch, which still seems to be needed.

Also, should this be removed ? Or at leat remove the parenthesized text, since
non-system tables no longer have OIDs: "(use to avoid output on system tables)"

https://www.postgresql.org/docs/devel/runtime-config-developer.html
trace_lock_oidmin (integer)

And maybe this (?)
trace_lock_table (integer)

On Wed, May 08, 2019 at 02:05:57PM -0500, Justin Pryzby wrote:

I found what appears to be a dangling reference to old "hidden" OID behavior.

Justin

From 1c6712c0ade949648dbc415dfd7ea80312360ef7 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Wed, 8 May 2019 13:57:12 -0500
Subject: [PATCH v1] Cleanup/remove/update references to OID column...

..in wake of 578b229718e8f.

See also
93507e67c9ca54026019ebec3026de35d30370f9
1464755fc490a9911214817fe83077a3689250ab
f6b39171f3d65155b9390c2c69bc5b3469f923a8

Author: Justin Pryzby <justin@telsasoft.com>
---
doc/src/sgml/catalogs.sgml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index d544e60..0f9c6f2 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -610,7 +610,7 @@
<entry><structfield>oid</structfield></entry>
<entry><type>oid</type></entry>
<entry></entry>
-      <entry>Row identifier (hidden attribute; must be explicitly selected)</entry>
+      <entry>Row identifier</entry>
</row>

<row>
--
2.7.4

--
Justin Pryzby
System Administrator
Telsasoft
+1-952-707-8581

Justin Pryzby <pryzby@telsasoft.com> writes:

I'm resending this patch, which still seems to be needed.

Yeah, clearly one copy of that text got missed out. Pushed that.

Also, should this be removed ? Or at leat remove the parenthesized text, since
non-system tables no longer have OIDs: "(use to avoid output on system tables)"

No, I think that's still fine as-is. Tables still have OIDs, they
just don't *contain* magic OID columns.

And maybe this (?)
trace_lock_table (integer)

Hm, the description of that isn't English, at least:

gettext_noop("Sets the OID of the table with unconditionally lock tracing."),

I'm not entirely sure about the point of tracing locks on just one
table, which seems to be what this is for.

regards, tom lane

Few comments seem to have dangling references to the behavior from pre-12 "WITH
OIDS". Maybe varsup.c should get a wider change?

diff --git a/src/backend/access/common/tupdesc.c b/src/backend/access/common/tupdesc.c
index 1e743d7d86..ce84e22cbd 100644
--- a/src/backend/access/common/tupdesc.c
+++ b/src/backend/access/common/tupdesc.c
@@ -786,9 +786,7 @@ TupleDescInitEntryCollation(TupleDesc desc,
  *
  * Given a relation schema (list of ColumnDef nodes), build a TupleDesc.
  *
- * Note: the default assumption is no OIDs; caller may modify the returned
- * TupleDesc if it wants OIDs.  Also, tdtypeid will need to be filled in
- * later on.
+ * tdtypeid will need to be filled in later on.
  */
 TupleDesc
 BuildDescForRelation(List *schema)
diff --git a/src/backend/access/transam/varsup.c b/src/backend/access/transam/varsup.c
index 2570e7086a..2f0eab0f53 100644
--- a/src/backend/access/transam/varsup.c
+++ b/src/backend/access/transam/varsup.c
@@ -527,8 +527,7 @@ GetNewObjectId(void)
 	 * The first time through this routine after normal postmaster start, the
 	 * counter will be forced up to FirstNormalObjectId.  This mechanism
 	 * leaves the OIDs between FirstBootstrapObjectId and FirstNormalObjectId
-	 * available for automatic assignment during initdb, while ensuring they
-	 * will never conflict with user-assigned OIDs.
+	 * available for automatic assignment during initdb.
 	 */
 	if (ShmemVariableCache->nextOid < ((Oid) FirstNormalObjectId))
 	{
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 262e14ccfb..1ca11960e2 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -4813,7 +4813,7 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode,
 			continue;

 		/*
-		 * If we change column data types or add/remove OIDs, the operation
+		 * If we change column data types, the operation
 		 * has to be propagated to tables that use this table's rowtype as a
 		 * column type.  tab->newvals will also be non-NULL in the case where
 		 * we're adding a column with a default.  We choose to forbid that
@@ -4837,8 +4837,7 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode,

 		/*
 		 * We only need to rewrite the table if at least one column needs to
-		 * be recomputed, we are adding/removing the OID column, or we are
-		 * changing its persistence.
+		 * be recomputed, or we are changing its persistence.
 		 *
 		 * There are two reasons for requiring a rewrite when changing
 		 * persistence: on one hand, we need to ensure that the buffers