Обсуждение: Why is specifying oids = false multiple times in create table is silently ignored?

Hi,

We generally throw an error when create table options are specified
more than once, see below:
postgres=# create table t1(a1 int) with (fillfactor = 10, fillfactor = 15);
ERROR:  parameter "fillfactor" specified more than once

Although "with oids" support is removed by the commit 578b229718 and
we do still support with (oids = false) as a no-op which may be for
backward compatibility. But, why do we need to allow specifying oids =
false multiple times(see below)? Shouldn't we throw an error for
consistency with other options?
postgres=# create table t1(a1 int) with (oids = false, oids = false,
oids = false);
CREATE TABLE

And also, the commit 578b229718 talks about removing "with (oids =
false)" someday. Is it the time now to remove that and error out with
"unrecognized parameter "oids""?
            /*
             * This is not a great place for this test, but there's no other
             * convenient place to filter the option out. As WITH (oids =
             * false) will be removed someday, this seems like an acceptable
             * amount of ugly.
             */
postgres=# create table t1(a1 int) with (oids = 10);
ERROR:  unrecognized parameter "oids"

Thoughts?

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Apr 07, 2021 at 04:00:46PM +0530, Bharath Rupireddy wrote:
> And also, the commit 578b229718 talks about removing "with (oids =
> false)" someday. Is it the time now to remove that and error out with
> "unrecognized parameter "oids""?

Nope, and I think that it will remain around for some time.  Keeping
around the code necessary to silence WITH OIDS has no real maintenance
cost, and removing it could easily break applications.  So there is
little gain in cleaning up that, and a lot of potential loss for
users.
--
Michael

On Wed, Apr 7, 2021 at 4:20 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Apr 07, 2021 at 04:00:46PM +0530, Bharath Rupireddy wrote:
> > And also, the commit 578b229718 talks about removing "with (oids =
> > false)" someday. Is it the time now to remove that and error out with
> > "unrecognized parameter "oids""?
>
> Nope, and I think that it will remain around for some time.  Keeping
> around the code necessary to silence WITH OIDS has no real maintenance
> cost, and removing it could easily break applications.  So there is
> little gain in cleaning up that, and a lot of potential loss for
> users.

I agree to not remove "with (oids = false)". At least shouldn't we fix
the "create table ... with (oids = false, oids = false ....)" case,
just to be consistent with other options?

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Apr 7, 2021 at 4:20 PM Michael Paquier <michael@paquier.xyz> wrote:

>

> On Wed, Apr 07, 2021 at 04:00:46PM +0530, Bharath Rupireddy wrote:

> > And also, the commit 578b229718 talks about removing "with (oids =

> > false)" someday. Is it the time now to remove that and error out with

> > "unrecognized parameter "oids""?

>

> Nope, and I think that it will remain around for some time.  Keeping

> around the code necessary to silence WITH OIDS has no real maintenance

> cost, and removing it could easily break applications.  So there is

> little gain in cleaning up that, and a lot of potential loss for

> users.

I agree to not remove "with (oids = false)". At least shouldn't we fix

the "create table ... with (oids = false, oids = false ....)" case,

just to be consistent with other options?

On Wed, Apr 07, 2021 at 11:09:41AM -0300, Euler Taveira wrote:
> On Wed, Apr 7, 2021, at 10:25 AM, Bharath Rupireddy wrote:
>> I agree to not remove "with (oids = false)". At least shouldn't we fix
>> the "create table ... with (oids = false, oids = false ....)" case,
>> just to be consistent with other options?
>
> It would be weird to error out while parsing a no-op option, no?

There is an argument to be made both ways here.

>> But, why do we need to allow specifying oids = false multiple times(see
>> below)? Shouldn't we throw an error for consistency with other options?
>>
>
> If you look at transformReloptions(), the no-op code is just a hack. Such a
> patch should add 'oids' as a reloption to test for multiple occurrences.
> Although, CREATE TABLE says you can use 'oids=false', Storage Parameters
> section does not mention it as a parameter. The code is fine as is.

But I agree with letting what we have here as it is, per the same
argument of upthread that this could just break stuff for free, and
that's not a maintenance burden either.
--
Michael

Hi,

On 2021-04-08 09:17:42 +0900, Michael Paquier wrote:
> On Wed, Apr 07, 2021 at 11:09:41AM -0300, Euler Taveira wrote:
> > On Wed, Apr 7, 2021, at 10:25 AM, Bharath Rupireddy wrote:
> >> I agree to not remove "with (oids = false)". At least shouldn't we fix
> >> the "create table ... with (oids = false, oids = false ....)" case,
> >> just to be consistent with other options?
> >
> > It would be weird to error out while parsing a no-op option, no?
> 
> There is an argument to be made both ways here.

> >> But, why do we need to allow specifying oids = false multiple times(see
> >> below)? Shouldn't we throw an error for consistency with other options?
> >>
> >
> > If you look at transformReloptions(), the no-op code is just a hack. Such a
> > patch should add 'oids' as a reloption to test for multiple occurrences.
> > Although, CREATE TABLE says you can use 'oids=false', Storage Parameters
> > section does not mention it as a parameter. The code is fine as is.
> 
> But I agree with letting what we have here as it is, per the same
> argument of upthread that this could just break stuff for free, and
> that's not a maintenance burden either.

Agreed.

Given that this case didn't error out before the OIDs removal, it seems
like it'd be really strange to make it error out in the compat code...

Greetings,

Andres Freund

On Thu, Apr 8, 2021 at 5:56 AM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2021-04-08 09:17:42 +0900, Michael Paquier wrote:
> > On Wed, Apr 07, 2021 at 11:09:41AM -0300, Euler Taveira wrote:
> > > On Wed, Apr 7, 2021, at 10:25 AM, Bharath Rupireddy wrote:
> > >> I agree to not remove "with (oids = false)". At least shouldn't we fix
> > >> the "create table ... with (oids = false, oids = false ....)" case,
> > >> just to be consistent with other options?
> > >
> > > It would be weird to error out while parsing a no-op option, no?
> >
> > There is an argument to be made both ways here.
>
> > >> But, why do we need to allow specifying oids = false multiple times(see
> > >> below)? Shouldn't we throw an error for consistency with other options?
> > >>
> > >
> > > If you look at transformReloptions(), the no-op code is just a hack. Such a
> > > patch should add 'oids' as a reloption to test for multiple occurrences.
> > > Although, CREATE TABLE says you can use 'oids=false', Storage Parameters
> > > section does not mention it as a parameter. The code is fine as is.
> >
> > But I agree with letting what we have here as it is, per the same
> > argument of upthread that this could just break stuff for free, and
> > that's not a maintenance burden either.
>
> Agreed.
>
> Given that this case didn't error out before the OIDs removal, it seems
> like it'd be really strange to make it error out in the compat code...

Agreed to not error out for a no-op case i.e. with (oids = false, oids
= false). Thank you all for providing thoughts. I'm ending the
discussion here.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com