Thread: Cleanup/remove/update references to OID column
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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