Re: [BUGS] Breakage with VACUUM ANALYSE + partitions

From: Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>
To: Andres Freund <andres(at)anarazel(dot)de>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, Thom Brown <thom(at)linux(dot)com>, Abhijit Menon-Sen <ams(at)2ndquadrant(dot)com>, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [BUGS] Breakage with VACUUM ANALYSE + partitions
Date: 2016-05-04 05:46:42
Message-ID: alpine.DEB.2.10.1605040727090.27782@sto
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-bugs pgsql-hackers


Hello Andres,

>>> An enum doesn't have a benefit for a bitmask imo - you can't "legally"
>>> use it as a type for functions accepting the bitmask.
>>
>> I do not understand. I suggested to use enum to enumerate the bitmask
>> constants, ISTM that it does not preclude to use it as a bitmask as you do,
>> it is just a replacement of the #define? The type constraint on the enum
>> does not disallow bitmasking values, I checked with both gcc & clang.
>
> There's not really a point in using an enum if you use neither the type
> (which you shouldn't because if you or the bitmask value you have types
> outside the range of the enum), nor the autogenerated numbers.

I do not think that there is such a constraint in C, you can use the enum
bitfield type, so you should. You can say in the type name that it is a
bitmask to make it clearer:

typedef enum {
EXTENSION_XXX = ...;
} extension_behavior_bitfield;

> Anyway seems fairly unimportant.

Sure, it is just cosmetic. Now the type is already an enum and you can
keep it that way, ISTM that it is cleaner to avoid defines, so I think you
should.

>> Then should it be _OPEN_INACTIVE[TED] or _OPEN_TRUNCATED rather than
>> _OPEN_DELETED, which is contradictory?
>
> Well, TRUNCATED doesn't entirely work, there's I think some cases where
> this currently also applies to deleted relations. I kind of like the
> unintuitive sounding name, because it's really a dangerous options (any
> further mdnblocks will be wrong).

Hmmm.

My issue is with the semantics of the name which implies how it can be
understand by someone reading the code. The interface deals with files, so
implicitely DELETED refers to files, and AFAICS no file was deleted. Maybe
rows in the relation where deleted somehow, or entries in an index, but
that has no sense at the API level. So I think you should not use DELETED.

I can see why a vaguely oxymoronic name is amusing, but I do not think
that it conveys any idea of "dangerous" as you suggest.

Now the important think is probably not that the code is the cleanest
possible one, but that it fixes the issue, so you should probably commit
something.

--
Fabien.

In response to

Responses

Browse pgsql-bugs by date

  From Date Subject
Next Message Pavel Golub 2016-05-04 06:16:46 Re: BUG #14064: Sort order of bytea, etc. not defined
Previous Message Andres Freund 2016-05-03 23:48:10 Re: [BUGS] Breakage with VACUUM ANALYSE + partitions

Browse pgsql-hackers by date

  From Date Subject
Next Message David G. Johnston 2016-05-04 05:59:05 Re: Make PG's "NOT NULL"s and attnotnull ("is_nullable") conform to SQL-2011
Previous Message Tom Lane 2016-05-04 05:19:37 Re: Make PG's "NOT NULL"s and attnotnull ("is_nullable") conform to SQL-2011