Thread: [HACKERS] Bizarre choice of case for RELKIND_PARTITIONED_TABLE
Is there a good reason why RELKIND_PARTITIONED_TABLE is 'P' not 'p'?
It looks rather out of place considering that seven of the eight
pre-existing relkind codes are lower case. (And no, I don't especially
approve of RELKIND_SEQUENCE being 'S' either, but it's far too late to
change that.) Also, in typical low-res monospaced fonts, there's nearly
no difference except vertical alignment between P and p, meaning that in
something like
regression=# select distinct relkind from pg_class;relkind
---------rtPvmiSc
(8 rows)
you have to look rather closely even to notice that what you're seeing
isn't in the case you might expect.
I think we should change this while we still can.
regards, tom lane
On Tue, Mar 7, 2017 at 12:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Is there a good reason why RELKIND_PARTITIONED_TABLE is 'P' not 'p'? > It looks rather out of place considering that seven of the eight > pre-existing relkind codes are lower case. (And no, I don't especially > approve of RELKIND_SEQUENCE being 'S' either, but it's far too late to > change that.) Also, in typical low-res monospaced fonts, there's nearly > no difference except vertical alignment between P and p, meaning that in > something like > > regression=# select distinct relkind from pg_class; > relkind > --------- > r > t > P > v > m > i > S > c > (8 rows) > > you have to look rather closely even to notice that what you're seeing > isn't in the case you might expect. > > I think we should change this while we still can. I can't muster a lot of outrage about this one way or another. One possible advantage of 'P' is that there are fewer places where 'P' is mentioned in the source code than 'p'. [rhaas pgsql]$ git grep "'p'" | wc -l 293 [rhaas pgsql]$ git grep "'P'" | wc -l 104 ...so it's a little easier to pick out the cases that are talking about partitioned tables than it would be with a lower case letter. However, as I say, I don't care very much. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Mar 7, 2017 at 12:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Is there a good reason why RELKIND_PARTITIONED_TABLE is 'P' not 'p'?
> I can't muster a lot of outrage about this one way or another. One
> possible advantage of 'P' is that there are fewer places where 'P' is
> mentioned in the source code than 'p'.
Hm, one would hope that the vast majority of code references are neither
of those, but rather "RELKIND_PARTITIONED_TABLE". information_schema.sql
and system_views.sql will need to be gone over carefully, certainly, but
we shouldn't be hard-coding this anywhere that there's a reasonable
alternative.
regards, tom lane
On 3/7/17 12:55, Tom Lane wrote: > Is there a good reason why RELKIND_PARTITIONED_TABLE is 'P' not 'p'? I was confused about this too. If there is no argument against it, I would favor changing it. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Mar 7, 2017 at 6:03 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Tue, Mar 7, 2017 at 12:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> Is there a good reason why RELKIND_PARTITIONED_TABLE is 'P' not 'p'? > >> I can't muster a lot of outrage about this one way or another. One >> possible advantage of 'P' is that there are fewer places where 'P' is >> mentioned in the source code than 'p'. > > Hm, one would hope that the vast majority of code references are neither > of those, but rather "RELKIND_PARTITIONED_TABLE". information_schema.sql > and system_views.sql will need to be gone over carefully, certainly, but > we shouldn't be hard-coding this anywhere that there's a reasonable > alternative. For reasons which must've seemed good to whoever instituted the policy, pg_dump refers to relkinds using the bare letters rather than the constants. (And protocol message types don't even have defined constants. Uggh.) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Mar 7, 2017 at 6:03 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Hm, one would hope that the vast majority of code references are neither
>> of those, but rather "RELKIND_PARTITIONED_TABLE".
> For reasons which must've seemed good to whoever instituted the
> policy, pg_dump refers to relkinds using the bare letters rather than
> the constants.
Even in pg_dump, it appears to me that the large majority of relkind
references use the symbolic names. Quite a few of the violations of
that policy look to be new ... and now that I see them, their days are
numbered.
> (And protocol message types don't even have defined constants. Uggh.)
Yeah, that's a different issue, which boils down to the fact that in order
to do anything useful we'd need to clutter client-visible namespace with
the symbols. I wouldn't be averse to doing something about it as long as
it's not done in postgres_ext.h, but if not there where?
regards, tom lane
On 2017/03/08 2:55, Tom Lane wrote: > Is there a good reason why RELKIND_PARTITIONED_TABLE is 'P' not 'p'? > It looks rather out of place considering that seven of the eight > pre-existing relkind codes are lower case. (And no, I don't especially > approve of RELKIND_SEQUENCE being 'S' either, but it's far too late to > change that.) Also, in typical low-res monospaced fonts, there's nearly > no difference except vertical alignment between P and p, meaning that in > something like > > regression=# select distinct relkind from pg_class; > relkind > --------- > r > t > P > v > m > i > S > c > (8 rows) > > you have to look rather closely even to notice that what you're seeing > isn't in the case you might expect. > > I think we should change this while we still can. I remember that one of the earliest versions of the patch I submitted had two new relkinds: 'P' for partitioned tables and 'p' for leaf partitions. The latter was dropped subsequently and I never thought of using 'p' instead of 'P' for partitioned tables. Attached patch that implements this change. Thanks, Amit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
I wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> For reasons which must've seemed good to whoever instituted the
>> policy, pg_dump refers to relkinds using the bare letters rather than
>> the constants.
> Even in pg_dump, it appears to me that the large majority of relkind
> references use the symbolic names.
After further study, I think it was psql/describe.c you were remembering.
I cleaned that up...
regards, tom lane
Tom Lane wrote: > (And no, I don't especially > approve of RELKIND_SEQUENCE being 'S' either, but it's far too late to > change that.) FWIW the reason SEQUENCE uses S instead of 's' is that the latter was taken for "special" relations, which we removed a few releases ago (commit 3a694bb0a1). -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Tom Lane wrote:
>> (And no, I don't especially
>> approve of RELKIND_SEQUENCE being 'S' either, but it's far too late to
>> change that.)
> FWIW the reason SEQUENCE uses S instead of 's' is that the latter was
> taken for "special" relations, which we removed a few releases ago
> (commit 3a694bb0a1).
Yeah, I'd just been reminded of that by some code in describe.c.
So there actually is a reason for sequences to be 'S', or once was.
regards, tom lane
I wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Tue, Mar 7, 2017 at 12:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Is there a good reason why RELKIND_PARTITIONED_TABLE is 'P' not 'p'?
>> I can't muster a lot of outrage about this one way or another. One
>> possible advantage of 'P' is that there are fewer places where 'P' is
>> mentioned in the source code than 'p'.
> Hm, one would hope that the vast majority of code references are neither
> of those, but rather "RELKIND_PARTITIONED_TABLE". information_schema.sql
> and system_views.sql will need to be gone over carefully, certainly, but
> we shouldn't be hard-coding this anywhere that there's a reasonable
> alternative.
Pushed. I was a bit disappointed to find that make check-world passed
just fine without having updated either information_schema.sql or
system_views.sql. Evidently our test coverage for these views leaves
something to be desired.
regards, tom lane
On Thu, Mar 9, 2017 at 11:06:56PM -0300, Alvaro Herrera wrote: > Tom Lane wrote: > > > (And no, I don't especially > > approve of RELKIND_SEQUENCE being 'S' either, but it's far too late to > > change that.) > > FWIW the reason SEQUENCE uses S instead of 's' is that the latter was > taken for "special" relations, which we removed a few releases ago > (commit 3a694bb0a1). Ah! I knew there was a reason for 'S', but couldn't remember it. :-) -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +