Horton

Thread

Commits

  1. Change the relkind for partitioned tables from 'P' to 'p'.

  2. Fix hard-coded relkind constants in assorted other files.

  3. Fix hard-coded relkind constants in assorted src/bin files.

  4. Fix hard-coded relkind constants in psql/describe.c.

  5. Fix hard-coded relkind constants in pg_dump.c.

  6. Restructure LOCKTAG as per discussions of a couple months ago.

  1. Bizarre choice of case for RELKIND_PARTITIONED_TABLE

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-03-07T17:55:19Z

    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.

			regards, tom lane

  2. Re: Bizarre choice of case for RELKIND_PARTITIONED_TABLE

    Robert Haas <robertmhaas@gmail.com> — 2017-03-07T19:25:55Z

    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

  3. Re: Bizarre choice of case for RELKIND_PARTITIONED_TABLE

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-03-07T23:03:38Z

    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

  4. Re: Bizarre choice of case for RELKIND_PARTITIONED_TABLE

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-03-07T23:31:26Z

    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

  5. Re: Bizarre choice of case for RELKIND_PARTITIONED_TABLE

    Robert Haas <robertmhaas@gmail.com> — 2017-03-07T23:43:10Z

    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

  6. Re: Bizarre choice of case for RELKIND_PARTITIONED_TABLE

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-03-08T00:02:04Z

    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

  7. Re: Bizarre choice of case for RELKIND_PARTITIONED_TABLE

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-03-08T03:54:16Z

    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

  8. Re: Bizarre choice of case for RELKIND_PARTITIONED_TABLE

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-03-10T01:54:24Z

    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

  9. Re: Bizarre choice of case for RELKIND_PARTITIONED_TABLE

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2017-03-10T02:06:56Z

    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

  10. Re: Bizarre choice of case for RELKIND_PARTITIONED_TABLE

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-03-10T02:10:58Z

    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

  11. Re: Bizarre choice of case for RELKIND_PARTITIONED_TABLE

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-03-10T18:16:05Z

    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

  12. Re: Bizarre choice of case for RELKIND_PARTITIONED_TABLE

    Bruce Momjian <bruce@momjian.us> — 2017-03-11T03:24:48Z

    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 +