Re: pgsql: Remove WITH OIDS support, change oid catalog column visibility.

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

Re: pgsql: Remove WITH OIDS support, change oid catalog column visibility.

Christoph Berg-2
Re: Andres Freund 2018-11-21 <[hidden email]>
> The biggest user of WITH OID columns was postgres' catalog. This
> commit changes all 'magic' oid columns to be columns that are normally
> declared and stored.

postgres=# \d+ pg_class
[...]
Indexe:
    "pg_class_oid_index" UNIQUE, btree (oid)

Now that oid is a proper column, shouldn't that be a PRIMARY KEY?
(Just for the looks.)

Christoph

Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Remove WITH OIDS support, change oid catalog column visibility.

Gavin Flower-2
On 21/11/2018 21:20, Christoph Berg wrote:

> Re: Andres Freund 2018-11-21 <[hidden email]>
>> The biggest user of WITH OID columns was postgres' catalog. This
>> commit changes all 'magic' oid columns to be columns that are normally
>> declared and stored.
> postgres=# \d+ pg_class
> [...]
> Indexe:
>      "pg_class_oid_index" UNIQUE, btree (oid)
>
> Now that oid is a proper column, shouldn't that be a PRIMARY KEY?
> (Just for the looks.)
>
> Christoph
>
Curious, is there a reason 'Index' is spelt with a trailing 'e'?


Cheers,
Gavin


Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Remove WITH OIDS support, change oid catalog column visibility.

Christoph Berg-2
Re: Gavin Flower 2018-11-21 <[hidden email]>
> Curious, is there a reason 'Index' is spelt with a trailing 'e'?

LANG=de_DE.UTF-8

Christoph

Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Remove WITH OIDS support, change oid catalog column visibility.

Peter Eisentraut-6
In reply to this post by Christoph Berg-2
On 21/11/2018 09:20, Christoph Berg wrote:

> Re: Andres Freund 2018-11-21 <[hidden email]>
>> The biggest user of WITH OID columns was postgres' catalog. This
>> commit changes all 'magic' oid columns to be columns that are normally
>> declared and stored.
>
> postgres=# \d+ pg_class
> [...]
> Indexe:
>     "pg_class_oid_index" UNIQUE, btree (oid)
>
> Now that oid is a proper column, shouldn't that be a PRIMARY KEY?
> (Just for the looks.)

This is an independent consideration.  There was nothing before that
prevented an index on an oid column from being a primary key that isn't
still there.

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Remove WITH OIDS support, change oid catalog column visibility.

Tom Lane-2
I noticed that this patch has broken restores of existing dump files:

psql:testbed.public.backup:82: ERROR:  unrecognized configuration parameter "default_with_oids"

Quite aside from the fact that this won't work if the user tries to
restore in single-transaction or no-error mode, this is really really
unhelpful because it's impossible to tell whether the error is
ignorable or not, except by digging through the dump file.

What you should do is restore that GUC, but add a check-hook that throws
an error for an attempt to set it to "true".

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Remove WITH OIDS support, change oid catalog column visibility.

Andres Freund
Hi,

On 2019-01-03 13:40:42 -0500, Tom Lane wrote:

> I noticed that this patch has broken restores of existing dump files:
>
> psql:testbed.public.backup:82: ERROR:  unrecognized configuration parameter "default_with_oids"
>
> Quite aside from the fact that this won't work if the user tries to
> restore in single-transaction or no-error mode, this is really really
> unhelpful because it's impossible to tell whether the error is
> ignorable or not, except by digging through the dump file.
>
> What you should do is restore that GUC, but add a check-hook that throws
> an error for an attempt to set it to "true".

Makes sense, I (or a colleague) will look into that next week. Wanna get my
head out of pluggable storage first...

Greetings,

Andres Freund

Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Remove WITH OIDS support, change oid catalog column visibility.

Amit Khandekar-2
On Sat, 5 Jan 2019 at 02:09, Andres Freund <[hidden email]> wrote:

>
> Hi,
>
> On 2019-01-03 13:40:42 -0500, Tom Lane wrote:
> > I noticed that this patch has broken restores of existing dump files:
> >
> > psql:testbed.public.backup:82: ERROR:  unrecognized configuration parameter "default_with_oids"
> >
> > Quite aside from the fact that this won't work if the user tries to
> > restore in single-transaction or no-error mode, this is really really
> > unhelpful because it's impossible to tell whether the error is
> > ignorable or not, except by digging through the dump file.
> >
> > What you should do is restore that GUC, but add a check-hook that throws
> > an error for an attempt to set it to "true".
Attached is a patch that does this.

>
> Makes sense, I (or a colleague) will look into that next week. Wanna get my
> head out of pluggable storage first...
>
> Greetings,
>
> Andres Freund
>


--
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company

errmsg_default_with_oids.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Remove WITH OIDS support, change oid catalog column visibility.

Andres Freund
Hi,

On 2019-01-07 17:03:20 +0530, Amit Khandekar wrote:

> On Sat, 5 Jan 2019 at 02:09, Andres Freund <[hidden email]> wrote:
> > On 2019-01-03 13:40:42 -0500, Tom Lane wrote:
> > > I noticed that this patch has broken restores of existing dump files:
> > >
> > > psql:testbed.public.backup:82: ERROR:  unrecognized configuration parameter "default_with_oids"
> > >
> > > Quite aside from the fact that this won't work if the user tries to
> > > restore in single-transaction or no-error mode, this is really really
> > > unhelpful because it's impossible to tell whether the error is
> > > ignorable or not, except by digging through the dump file.
> > >
> > > What you should do is restore that GUC, but add a check-hook that throws
> > > an error for an attempt to set it to "true".
>
> Attached is a patch that does this.

Thanks!  I've pushed this, after some minor editorialization (hiding the
GUC, shortened description, shortened error message).

Regards,

Andres