Thread: [HACKERS] Should pg_current_wal_location() become pg_current_wal_lsn()

... and of course the other functions matching *wal*location*

My thoughts here are that we're already breaking backward
compatibility of these functions for PG10, so thought we might want to
use this as an opportunity to fix the naming a bit more.

I feel that the "location" word not the best choice.  We also have a
function called pg_tablespace_location() to give us the path that a
tablespace is stored in, so I could understand anyone who's confused
about what pg_current_wal_location() might do if they're looking at
the function name only, and not the docs.

For me, "lsn" suits these function names much better, so I'd like to
see what other's think.

It would be sad to miss this opportunity without at least discussing this first.

The functions in question are:

postgres=# \dfS *wal*location*                                      List of functions  Schema   |              Name
        | Result data type |
 
Argument data types |  Type
------------+--------------------------------+------------------+---------------------+--------pg_catalog |
pg_current_wal_flush_location | pg_lsn           |              | normalpg_catalog | pg_current_wal_insert_location |
pg_lsn          |              | normalpg_catalog | pg_current_wal_location        | pg_lsn           |              |
normalpg_catalog| pg_last_wal_receive_location   | pg_lsn           |              | normalpg_catalog |
pg_last_wal_replay_location   | pg_lsn           |              | normalpg_catalog | pg_wal_location_diff           |
numeric         |
 
pg_lsn, pg_lsn      | normal
(6 rows)

Opinions?

-- David Rowley                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services

At Mon, 10 Apr 2017 19:26:11 +1200, David Rowley <david.rowley@2ndquadrant.com> wrote in
<CAKJS1f8O0njDKe8ePFQ-LK5-EjwThsDws6ohJ-+c6nWK+oUxtg@mail.gmail.com>
> ... and of course the other functions matching *wal*location*
> 
> My thoughts here are that we're already breaking backward
> compatibility of these functions for PG10, so thought we might want to
> use this as an opportunity to fix the naming a bit more.
> 
> I feel that the "location" word not the best choice.  We also have a
> function called pg_tablespace_location() to give us the path that a
> tablespace is stored in, so I could understand anyone who's confused
> about what pg_current_wal_location() might do if they're looking at
> the function name only, and not the docs.
> 
> For me, "lsn" suits these function names much better, so I'd like to
> see what other's think.
> 
> It would be sad to miss this opportunity without at least discussing this first.
> 
> The functions in question are:
> 
> postgres=# \dfS *wal*location*
>                                        List of functions
>    Schema   |              Name              | Result data type |
> Argument data types |  Type
> ------------+--------------------------------+------------------+---------------------+--------
>  pg_catalog | pg_current_wal_flush_location  | pg_lsn           |
>                | normal
>  pg_catalog | pg_current_wal_insert_location | pg_lsn           |
>                | normal
>  pg_catalog | pg_current_wal_location        | pg_lsn           |
>                | normal
>  pg_catalog | pg_last_wal_receive_location   | pg_lsn           |
>                | normal
>  pg_catalog | pg_last_wal_replay_location    | pg_lsn           |
>                | normal
>  pg_catalog | pg_wal_location_diff           | numeric          |
> pg_lsn, pg_lsn      | normal
> (6 rows)
> 
> Opinions?

Similariliy, these columns may need renaming.

s=# select attname, attrelid::regclass from pg_attribute where attname like '%location%';    attname     |
attrelid      
 
-----------------+---------------------sent_location   | pg_stat_replicationwrite_location  |
pg_stat_replicationflush_location | pg_stat_replicationreplay_location | pg_stat_replication
 
(4 rows)


Currently the following functions and columns are using 'lsn'.

=# \dfS *lsn*                            List of functions  Schema   |    Name     | Result data type | Argument data
types|  Type  
 
------------+-------------+------------------+---------------------+--------pg_catalog | pg_lsn_cmp  | integer
|pg_lsn, pg_lsn      | normalpg_catalog | pg_lsn_eq   | boolean          | pg_lsn, pg_lsn      | normal
 
...pg_catalog | pg_lsn_recv | pg_lsn           | internal            | normalpg_catalog | pg_lsn_send | bytea
| pg_lsn              | normal
 
(13 rows)


=# select distinct attname from pg_attribute where attname like '%lsn%';      attname       

---------------------confirmed_flush_lsnlatest_end_lsnlocal_lsnreceive_start_lsnreceived_lsnremote_lsnrestart_lsnsrsublsn
(8 rows)


Feature is already frozen, but this seems inconsistent a bit..

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Fri, Apr 14, 2017 at 4:28 AM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> Similariliy, these columns may need renaming.
>
> s=# select attname, attrelid::regclass from pg_attribute where attname like '%location%';
>      attname     |      attrelid
> -----------------+---------------------
>  sent_location   | pg_stat_replication
>  write_location  | pg_stat_replication
>  flush_location  | pg_stat_replication
>  replay_location | pg_stat_replication
> (4 rows)

Personally, I would be inclined not to tinker with this, not just
because we're after freeze but because it doesn't seem like an
improvement to me.  Referring to an LSN as  location seems fine to me;
I mean, granted, it's one specific kind of location, but that doesn't
make it wrong.  If somebody calls you and says "let's meet up", and
you say "sure, what's your location?" they might give you a street
address or GPS coordinates or the name of a nearby point of interest,
depending on what information they have easily available, but that's
cool; those things all let you find them.  Similarly, an LSN lets you
find a particular point within a WAL stream, but I don't think that
makes it not a location.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Apr 14, 2017 at 4:28 AM, Kyotaro HORIGUCHI
> <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
>> Similariliy, these columns may need renaming.

> Personally, I would be inclined not to tinker with this, not just
> because we're after freeze but because it doesn't seem like an
> improvement to me.  Referring to an LSN as  location seems fine to me;
> I mean, granted, it's one specific kind of location, but that doesn't
> make it wrong.

In a green field it would be perfectly fine --- but I think Kyotaro-san's
point is about consistency.  If all the other exposed names that involve
the same concept use "lsn", then it's fair to say that it's a bad idea
for these four column names to be randomly different from the rest.

Now this is a pre-existing problem: those column names existed in 9.6,
and so did some of the ones named using "lsn".  But we've added more
of the latter in v10.  I think the real problem right now is that nobody
has a rule to follow about which way to name new exposed references to
the concept, and that's simply bad.

It's possible that we should say that backwards compatibility outweighs
consistency and therefore it's too late to change these names.  But
I think your argument above is missing the point.
        regards, tom lane

On Fri, Apr 14, 2017 at 10:22:36AM -0400, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Fri, Apr 14, 2017 at 4:28 AM, Kyotaro HORIGUCHI
> > <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> >> Similariliy, these columns may need renaming.
> 
> > Personally, I would be inclined not to tinker with this, not just
> > because we're after freeze but because it doesn't seem like an
> > improvement to me.  Referring to an LSN as  location seems fine to me;
> > I mean, granted, it's one specific kind of location, but that doesn't
> > make it wrong.
> 
> In a green field it would be perfectly fine --- but I think Kyotaro-san's
> point is about consistency.  If all the other exposed names that involve
> the same concept use "lsn", then it's fair to say that it's a bad idea
> for these four column names to be randomly different from the rest.
> 
> Now this is a pre-existing problem: those column names existed in 9.6,
> and so did some of the ones named using "lsn".  But we've added more
> of the latter in v10.  I think the real problem right now is that nobody
> has a rule to follow about which way to name new exposed references to
> the concept, and that's simply bad.
> 
> It's possible that we should say that backwards compatibility outweighs
> consistency and therefore it's too late to change these names.  But
> I think your argument above is missing the point.

Yeah, this area is complex enough so any consistency we can add helps.

--  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 +

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Fri, Apr 14, 2017 at 4:28 AM, Kyotaro HORIGUCHI
> > <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> >> Similariliy, these columns may need renaming.
>
> > Personally, I would be inclined not to tinker with this, not just
> > because we're after freeze but because it doesn't seem like an
> > improvement to me.  Referring to an LSN as  location seems fine to me;
> > I mean, granted, it's one specific kind of location, but that doesn't
> > make it wrong.
>
> In a green field it would be perfectly fine --- but I think Kyotaro-san's
> point is about consistency.  If all the other exposed names that involve
> the same concept use "lsn", then it's fair to say that it's a bad idea
> for these four column names to be randomly different from the rest.
>
> Now this is a pre-existing problem: those column names existed in 9.6,
> and so did some of the ones named using "lsn".  But we've added more
> of the latter in v10.  I think the real problem right now is that nobody
> has a rule to follow about which way to name new exposed references to
> the concept, and that's simply bad.
>
> It's possible that we should say that backwards compatibility outweighs
> consistency and therefore it's too late to change these names.  But
> I think your argument above is missing the point.

I agree and definitely view 'lsn' as better than just 'location' when
we're talking about an lsn.  The datatype is 'pg_lsn', let's use 'lsn'
whenever that's what it is.  Consistency here is really good.

Thanks!

Stephen

On 4/14/17 04:28, Kyotaro HORIGUCHI wrote:
> =# select distinct attname from pg_attribute where attname like '%lsn%';
>        attname       
> ---------------------
>  confirmed_flush_lsn
>  latest_end_lsn
>  local_lsn
>  receive_start_lsn
>  received_lsn
>  remote_lsn
>  restart_lsn
>  srsublsn
> (8 rows)
> 
> 
> Feature is already frozen, but this seems inconsistent a bit..

I think these are all recently added for logical replication.  We could
rename them to _location.

I'm not a fan of renaming everything the opposite way.

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

On 4/14/17 11:36, Bruce Momjian wrote:
> Yeah, this area is complex enough so any consistency we can add helps.

If we're talking about making things easier to understand, wouldn't a
random user rather know what a WAL "location" is instead of a WAL "LSN"?

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

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> If we're talking about making things easier to understand, wouldn't a
> random user rather know what a WAL "location" is instead of a WAL "LSN"?

I wouldn't object to standardizing on "location" instead of "lsn" in the
related function and column names.  What I don't like is using different
words for the same thing.
        regards, tom lane

On Fri, Apr 14, 2017 at 6:52 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>> If we're talking about making things easier to understand, wouldn't a
>> random user rather know what a WAL "location" is instead of a WAL "LSN"?
>
> I wouldn't object to standardizing on "location" instead of "lsn" in the
> related function and column names.  What I don't like is using different
> words for the same thing.

The case mentioned in the subject of this thread has been using the
word "location" since time immemorial.  It's true that we've already
renamed it (xlog -> wal) in this release, so if we want to standardize
on lsn, now's certainly the time to do it.  I'm worried that
pg_current_wal_lsn() is an identifier composed almost entirely of
abbreviations and therefore possibly just as impenetrable as
qx_current_pfq_dnr(), but maybe we should assume that LSN is a term of
art with which knowledgeable users are required to be familiar, much
as we are doing for "WAL".

It appears, from grepping the 9.6 version of pg_proc.h, that both lsn
and location have some historical precedent.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

At Fri, 14 Apr 2017 18:26:37 -0400, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote in
<052f4ce0-159a-1666-f136-91977d3267a5@2ndquadrant.com>
> On 4/14/17 04:28, Kyotaro HORIGUCHI wrote:
> > =# select distinct attname from pg_attribute where attname like '%lsn%';
> >        attname       
> > ---------------------
> >  confirmed_flush_lsn
> >  latest_end_lsn
> >  local_lsn
> >  receive_start_lsn
> >  received_lsn
> >  remote_lsn
> >  restart_lsn
> >  srsublsn
> > (8 rows)
> > 
> > 
> > Feature is already frozen, but this seems inconsistent a bit..
> 
> I think these are all recently added for logical replication.  We could
> rename them to _location.
> 
> I'm not a fan of renaming everything the opposite way.

I don't particulary care for either. What is most unpleasant here
for me is the inconsistency among several replication-related
tables. Logical replication stuff is using LSN and physical sutff
has been using location, but pg_stat_wal_receiver is using
LSN. pg_replication_slots as the common stuff is using LSN.

"Location" fits attribute names since the table name implies that
the location is "LSN".

On the other hand nothing suggests such implication on function
names. So only "wal_location" or "lsn" can be used in function
names. pg_current_wal_* requires to be "wal_lsn" even using LSN
since "LSN" itself doesn't imply WAL files being
written. "wal_lsn" looks somewhat too-much, though.

Columns:
=# select attrelid::regclass || '.' || attname from pg_attribute where attname like '%location%' or attname like
'%lsn%';               ?column?                 
 

------------------------------------------pg_subscription_rel.srsublsnpg_stat_replication.sent_locationpg_stat_replication.write_locationpg_stat_replication.flush_locationpg_stat_replication.replay_locationpg_stat_wal_receiver.receive_start_lsnpg_stat_wal_receiver.received_lsnpg_stat_wal_receiver.latest_end_lsnpg_stat_subscription.received_lsnpg_stat_subscription.latest_end_lsnpg_replication_slots.restart_lsnpg_replication_slots.confirmed_flush_lsnpg_replication_origin_status.remote_lsnpg_replication_origin_status.local_lsn


pg_subscription_rel has a bit different naming convention from
others. But I'm not sure that involving it in the unification is
good since it doesn't seem to be explicitly exposed to users.


=# select proname from pg_proc where proname like '%location%' or proname like '%lsn%';           proname             
--------------------------------pg_tablespace_location     ## This is
irrelevantpg_current_wal_locationpg_current_wal_insert_locationpg_current_wal_flush_locationpg_wal_location_diffpg_last_wal_receive_locationpg_last_wal_replay_locationpg_lsn_mipg_lsn_inpg_lsn_outpg_lsn_ltpg_lsn_lepg_lsn_eqpg_lsn_gepg_lsn_gtpg_lsn_nepg_lsn_recvpg_lsn_sendpg_lsn_cmppg_lsn_hash

I think we can use "location" for all attributes and functions
except pg_lsn operators.

The last annoyance would be pg_wal_location_diff(). This exists
only for backward compatibility but the name 'pg_wal_lsn_diff' is
already so far from the original name that it becomes totally
useless.

Any more thoughts?

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

At Sat, 15 Apr 2017 12:56:32 -0400, Robert Haas <robertmhaas@gmail.com> wrote in
<CA+TgmoZjDo9ckxf6aYrqyMoiSw5yfBB2gpMbrBtE9zr==uczhw@mail.gmail.com>
> On Fri, Apr 14, 2017 at 6:52 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> >> If we're talking about making things easier to understand, wouldn't a
> >> random user rather know what a WAL "location" is instead of a WAL "LSN"?
> >
> > I wouldn't object to standardizing on "location" instead of "lsn" in the
> > related function and column names.  What I don't like is using different
> > words for the same thing.
> 
> The case mentioned in the subject of this thread has been using the
> word "location" since time immemorial.  It's true that we've already
> renamed it (xlog -> wal) in this release, so if we want to standardize
> on lsn, now's certainly the time to do it.  I'm worried that
> pg_current_wal_lsn() is an identifier composed almost entirely of
> abbreviations and therefore possibly just as impenetrable as
> qx_current_pfq_dnr(), but maybe we should assume that LSN is a term of
> art with which knowledgeable users are required to be familiar, much
> as we are doing for "WAL".
> 
> It appears, from grepping the 9.6 version of pg_proc.h, that both lsn
> and location have some historical precedent.

I'd better to have replied here. The detail is in my reply on
another brandh of this thread.

https://www.postgresql.org/message-id/20170417.143937.232025253.horiguchi.kyotaro@lab.ntt.co.jp

After all, "location" seems better to me in user interface. We
could rename almost all of %lsn% names into %location% except
pg_lsn oprators as long as it doesn't become too long or complex.

One annoyance is the historical function pg_xlog_location_diff(),
which is currently just an alias of pg_lsn_mi. It is
substantially an operator, but 'pg_wal_lsn_diff()' is so far from
the historical name that it becomes totally useless.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On 4/15/17 12:56 PM, Robert Haas wrote:
> On Fri, Apr 14, 2017 at 6:52 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>>> If we're talking about making things easier to understand, wouldn't a
>>> random user rather know what a WAL "location" is instead of a WAL "LSN"?
>> I wouldn't object to standardizing on "location" instead of "lsn" in the
>> related function and column names.  What I don't like is using different
>> words for the same thing.
> The case mentioned in the subject of this thread has been using the
> word "location" since time immemorial.  It's true that we've already
> renamed it (xlog -> wal) in this release, so if we want to standardize
> on lsn, now's certainly the time to do it.  I'm worried that
> pg_current_wal_lsn() is an identifier composed almost entirely of
> abbreviations and therefore possibly just as impenetrable as
> qx_current_pfq_dnr(), but maybe we should assume that LSN is a term of
> art with which knowledgeable users are required to be familiar, much
> as we are doing for "WAL".

+1, and I'm in favor of using "lsn" wherever applicable.  It seems to me
that if a user calls a function (or queries a table) that returns an lsn
then they should be aware of what an lsn is.

-- 
-David
david@pgmasters.net

On 19 April 2017 at 03:33, David Steele <david@pgmasters.net> wrote:
> +1, and I'm in favor of using "lsn" wherever applicable.  It seems to me
> that if a user calls a function (or queries a table) that returns an lsn
> then they should be aware of what an lsn is.

OK, so I've read over this thread again and I think it's time to
summarise the votes:

It seem that;

Robert mentions he'd be inclined not to tinker with this, but mentions
nothing of inconsistency.
Bruce mentions he'd like consistency but does not mention which he'd prefer.
Stephen wants consistency and votes to change "location" to "lsn" in
regards to a pg_lsn.
Peter would rather see use of "location", mentions about changing the
new in v10 stuff, but not about the existing 9.6 usages of lsn in
column names
Tom would not object to use of "location" over "lsn".
Kyotaro would rather see the use of "location" for all apart from the
pg_lsn operator functions. Unsure how we handle pg_wal_location_diff()
David Steel would rather see this changed to use "lsn" instead of location.


So in summary, nobody apart from Robert appeared to have any concerns
over breaking backward compatibility.

In favour of "location" -> "lsn": Stephen, David Steel,
In favour of "lsn" -> "location": Peter, Tom, Kyotaro

I left Bruce out since he just voted for consistency.

So "lsn" -> "location" seems to be winning

Is that enough to proceed?

Anyone else?

The patch to do this should likely also include a regression test to
ensure nothing new creeps in which breaks the new standard.

-- David Rowley                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services

David Rowley <david.rowley@2ndquadrant.com> writes:
> OK, so I've read over this thread again and I think it's time to
> summarise the votes:
> ...
> In favour of "location" -> "lsn": Stephen, David Steel,
> In favour of "lsn" -> "location": Peter, Tom, Kyotaro

FWIW, I was not voting in favor of "location"; I was just saying that
I wanted consistency.  If we're voting which way to move, please count
me as a vote for "lsn".
        regards, tom lane

On 19 April 2017 at 15:31, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> David Rowley <david.rowley@2ndquadrant.com> writes:
>> OK, so I've read over this thread again and I think it's time to
>> summarise the votes:
>> ...
>> In favour of "location" -> "lsn": Stephen, David Steel,
>> In favour of "lsn" -> "location": Peter, Tom, Kyotaro
>
> FWIW, I was not voting in favor of "location"; I was just saying that
> I wanted consistency.  If we're voting which way to move, please count
> me as a vote for "lsn".

Updated votes:

In favour of "location" -> "lsn": Tom, Stephen, David Steel
In favour of "lsn" -> "location": Peter, Kyotaro

-- David Rowley                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Apr 19, 2017 at 12:36 PM, David Rowley
<david.rowley@2ndquadrant.com> wrote:
> In favour of "location" -> "lsn": Tom, Stephen, David Steel
> In favour of "lsn" -> "location": Peter, Kyotaro

I vote for "location" -> "lsn". I would expect complains about the
current inconsistency at some point, and the function names have been
already changed for this release..
-- 
Michael

At Wed, 19 Apr 2017 13:32:48 +0900, Michael Paquier <michael.paquier@gmail.com> wrote in
<CAB7nPqR=jHB2Eh0r6bjcExsU_qkdWFyo23coxBt325aHmcSiuw@mail.gmail.com>
> On Wed, Apr 19, 2017 at 12:36 PM, David Rowley
> <david.rowley@2ndquadrant.com> wrote:
> > In favour of "location" -> "lsn": Tom, Stephen, David Steel
> > In favour of "lsn" -> "location": Peter, Kyotaro
> 
> I vote for "location" -> "lsn". I would expect complains about the
> current inconsistency at some point, and the function names have been
> already changed for this release..

I won't stick on "location" except that "pg_wal_lsn_diff" seems
no longer useful.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On 20 April 2017 at 07:29, Euler Taveira <euler@timbira.com.br> wrote:
> 2017-04-19 1:32 GMT-03:00 Michael Paquier <michael.paquier@gmail.com>:
>>
>> I vote for "location" -> "lsn". I would expect complains about the
>> current inconsistency at some point, and the function names have been
>> already changed for this release..

OK, so I've created a draft patch which does this.

In summary of what it does

1. Renames all wal_location functions to wal_lsn.
2. Renames all system view columns to use "lsn" instead of "location"
3. Rename function parameters to use "lsn" instead of "location".
4. Rename function parameters "wal_position" to "lsn". (Not mentioned
before, but seems consistency was high on the list from earlier
comments on the thread)
5. Change documentation to reflect the above changes.
6. Fix bug where docs claimed return type of
pg_logical_slot_peek_changes.location was text, when it was pg_lsn
(maybe apply separately?)
7. Change some places in the func.sgml where it was referring to the
lsn as a "position" rather than "location". (We want consistency here)

Is this touching all places which were mentioned by everyone?

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2 May 2017 at 00:10, David Rowley <david.rowley@2ndquadrant.com> wrote:
> On 20 April 2017 at 07:29, Euler Taveira <euler@timbira.com.br> wrote:
>> 2017-04-19 1:32 GMT-03:00 Michael Paquier <michael.paquier@gmail.com>:
>>>
>>> I vote for "location" -> "lsn". I would expect complains about the
>>> current inconsistency at some point, and the function names have been
>>> already changed for this release..
>
> OK, so I've created a draft patch which does this.

I ended up adding this to the open items list.  I feel it's best to be
on there so that we don't forget about this.

If we decide not to do it then we can just remove it from the list,
but it would be a shame to release the beta having forgotten to make
this change.

Do any of the committers who voted for this change feel inclined to
pick this patch up?

-- David Rowley                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services

On Fri, May 05, 2017 at 03:36:39AM +1200, David Rowley wrote:
> On 2 May 2017 at 00:10, David Rowley <david.rowley@2ndquadrant.com> wrote:
> > On 20 April 2017 at 07:29, Euler Taveira <euler@timbira.com.br> wrote:
> >> 2017-04-19 1:32 GMT-03:00 Michael Paquier <michael.paquier@gmail.com>:
> >>>
> >>> I vote for "location" -> "lsn". I would expect complains about the
> >>> current inconsistency at some point, and the function names have been
> >>> already changed for this release..
> >
> > OK, so I've created a draft patch which does this.
> 
> I ended up adding this to the open items list.  I feel it's best to be
> on there so that we don't forget about this.
> 
> If we decide not to do it then we can just remove it from the list,
> but it would be a shame to release the beta having forgotten to make
> this change.
> 
> Do any of the committers who voted for this change feel inclined to
> pick this patch up?

I'll echo that question.  This open item lacks a clear owner.  One might argue
that 806091c caused it by doing the backward-compatibility breaks that
inspired this patch, but that's a link too tenuous to create ownership.

Noah Misch <noah@leadboat.com> writes:
> On Fri, May 05, 2017 at 03:36:39AM +1200, David Rowley wrote:
>> Do any of the committers who voted for this change feel inclined to
>> pick this patch up?

> I'll echo that question.  This open item lacks a clear owner.  One might argue
> that 806091c caused it by doing the backward-compatibility breaks that
> inspired this patch, but that's a link too tenuous to create ownership.

If no one else takes this, I will pick it up --- but I don't anticipate
having any time for it until after Monday's back-branch release wraps.
        regards, tom lane

On 5/1/17 08:10, David Rowley wrote:
> On 20 April 2017 at 07:29, Euler Taveira <euler@timbira.com.br> wrote:
>> 2017-04-19 1:32 GMT-03:00 Michael Paquier <michael.paquier@gmail.com>:
>>>
>>> I vote for "location" -> "lsn". I would expect complains about the
>>> current inconsistency at some point, and the function names have been
>>> already changed for this release..
> 
> OK, so I've created a draft patch which does this.

After reading this patch, I see that

a) The scope of the compatibility break is expanded significantly beyond
what was already affected by the xlog->wal renaming.

b) Generally, things read less nicely and look more complicated.

So I still think we'd be better off leaving things the way they are.

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

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 5/1/17 08:10, David Rowley wrote:
>> OK, so I've created a draft patch which does this.

> After reading this patch, I see that
> a) The scope of the compatibility break is expanded significantly beyond
> what was already affected by the xlog->wal renaming.
> b) Generally, things read less nicely and look more complicated.

> So I still think we'd be better off leaving things the way they are.

What I find in a bit of looking around is that

1. There are no functions named *lsn* except the support functions
for the pg_lsn type.

2. There are half a dozen or so functions named *location* (the
ones hit in this patch).  All of them existed in 9.6, but with
names involving "xlog"; they're already renamed to involve "wal".

3. We have these system columns named *lsn*:

regression=# select attrelid::regclass, attname, atttypid::regtype from pg_attribute where attname like '%lsn%' and
attrelid<16384;         attrelid           |       attname       | atttypid  
------------------------------+---------------------+----------pg_stat_wal_receiver         | receive_start_lsn   |
pg_lsnpg_stat_wal_receiver        | received_lsn        | pg_lsnpg_stat_wal_receiver         | latest_end_lsn      |
pg_lsnpg_replication_slots        | restart_lsn         | pg_lsnpg_replication_slots         | confirmed_flush_lsn |
pg_lsnpg_replication_origin_status| remote_lsn          | pg_lsnpg_replication_origin_status | local_lsn           |
pg_lsnpg_subscription_rel         | srsublsn            | pg_lsnpg_stat_subscription         | received_lsn        |
pg_lsnpg_stat_subscription        | latest_end_lsn      | pg_lsn 
(10 rows)

The first seven of those existed in 9.6.

4. We have these system columns named *location*:

regression=# select attrelid::regclass, attname, atttypid::regtype from pg_attribute where attname like '%location%'
andattrelid<16384;     attrelid       |     attname     | atttypid  
---------------------+-----------------+----------pg_stat_replication | sent_location   | pg_lsnpg_stat_replication |
write_location | pg_lsnpg_stat_replication | flush_location  | pg_lsnpg_stat_replication | replay_location | pg_lsn 
(4 rows)

All of those existed in 9.6.  The patch proposes to rename them to *lsn.


So it seems like we do not have such a big problem with function names:
the relevant functions all use "location" in their names, and we could
just keep doing that.  The problem that we've got is inconsistency in
table/view column names.  However, there is no way to fix it without
adding a new dollop of compatibility break on top of what we've done
already.

For completeness, I think the available alternatives are:

#1: Leave these names as they stand.

#2: Rename all these functions and columns to "lsn", as in this patch.

#3: Rename the functions but not the columns.

#4: Leave the function names alone, standardize on "lsn" in column names
(hence, touching pg_stat_replication only).

#5: Standardize on "location" not "lsn" (hence, leaving the function
names alone, and touching pg_stat_wal_receiver, pg_replication_slots,
and pg_replication_origin_status, as well as the new-in-v10-anyway
pg_subscription_rel and pg_stat_subscription).

#3 has the advantage of not breaking anything we didn't break already,
but it isn't accomplishing very much in terms of improving consistency.
With #4, we have a rule that function names use "location" while column
names use "lsn", which is a bit odd but not terrible.
I think #5 would best serve Peter's point about readability, because
I agree that "lsn" isn't doing us any favors in that direction.

So this boils down to whether we are willing to touch any of these
column names in order to improve consistency.  I think it might be
worth doing, but there's no doubt that we're adding more compatibility
pain if we do anything but #1 or #3.
        regards, tom lane

PS: There are some other changes in David's patch, such as
s/position/location/ in some text, that I think we should do in any
case.  But the first decision has to be about the view column names.

Tom, all,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> > On 5/1/17 08:10, David Rowley wrote:
> >> OK, so I've created a draft patch which does this.
>
> > After reading this patch, I see that
> > a) The scope of the compatibility break is expanded significantly beyond
> > what was already affected by the xlog->wal renaming.

Changing one additional view doesn't strike me as a significantly
expanded set.

> > b) Generally, things read less nicely and look more complicated.

I disagree.

> > So I still think we'd be better off leaving things the way they are.
>
> What I find in a bit of looking around is that
>
> 1. There are no functions named *lsn* except the support functions
> for the pg_lsn type.

Even so, that doesn't strike me as having been particularly intentional
or because it was "better" to use something else, especially given the
column naming.

> 2. There are half a dozen or so functions named *location* (the
> ones hit in this patch).  All of them existed in 9.6, but with
> names involving "xlog"; they're already renamed to involve "wal".

Right, so changing location -> lsn for those doesn't expand the
compatibility issue really.

> 3. We have these system columns named *lsn*:
>
> regression=# select attrelid::regclass, attname, atttypid::regtype from pg_attribute where attname like '%lsn%' and
attrelid<16384;
>            attrelid           |       attname       | atttypid
> ------------------------------+---------------------+----------
>  pg_stat_wal_receiver         | receive_start_lsn   | pg_lsn
>  pg_stat_wal_receiver         | received_lsn        | pg_lsn
>  pg_stat_wal_receiver         | latest_end_lsn      | pg_lsn
>  pg_replication_slots         | restart_lsn         | pg_lsn
>  pg_replication_slots         | confirmed_flush_lsn | pg_lsn
>  pg_replication_origin_status | remote_lsn          | pg_lsn
>  pg_replication_origin_status | local_lsn           | pg_lsn
>  pg_subscription_rel          | srsublsn            | pg_lsn
>  pg_stat_subscription         | received_lsn        | pg_lsn
>  pg_stat_subscription         | latest_end_lsn      | pg_lsn
> (10 rows)
>
> The first seven of those existed in 9.6.
>
> 4. We have these system columns named *location*:
>
> regression=# select attrelid::regclass, attname, atttypid::regtype from pg_attribute where attname like '%location%'
andattrelid<16384; 
>       attrelid       |     attname     | atttypid
> ---------------------+-----------------+----------
>  pg_stat_replication | sent_location   | pg_lsn
>  pg_stat_replication | write_location  | pg_lsn
>  pg_stat_replication | flush_location  | pg_lsn
>  pg_stat_replication | replay_location | pg_lsn
> (4 rows)
>
> All of those existed in 9.6.  The patch proposes to rename them to *lsn.

Right, four column names in one view- the one view which is different
from all of the other views that exist.

> So it seems like we do not have such a big problem with function names:
> the relevant functions all use "location" in their names, and we could
> just keep doing that.  The problem that we've got is inconsistency in
> table/view column names.  However, there is no way to fix it without
> adding a new dollop of compatibility break on top of what we've done
> already.
>
> For completeness, I think the available alternatives are:
>
> #1: Leave these names as they stand.

-1

> #2: Rename all these functions and columns to "lsn", as in this patch.

+1

> #3: Rename the functions but not the columns.

-1

> #4: Leave the function names alone, standardize on "lsn" in column names
> (hence, touching pg_stat_replication only).

-1

> #5: Standardize on "location" not "lsn" (hence, leaving the function
> names alone, and touching pg_stat_wal_receiver, pg_replication_slots,
> and pg_replication_origin_status, as well as the new-in-v10-anyway
> pg_subscription_rel and pg_stat_subscription).

Ugh, that seems even worse, and expands the compatibility breakage, -5.

> #3 has the advantage of not breaking anything we didn't break already,
> but it isn't accomplishing very much in terms of improving consistency.

Agreed.

> With #4, we have a rule that function names use "location" while column
> names use "lsn", which is a bit odd but not terrible.

Given that we're changing the function names already, I really don't see
why this makes any sense.  Perhaps it's just me, but 'location' is much
more of a vague term than 'lsn' and if we're talking about an 'lsn' then
that's what we should be using.

> I think #5 would best serve Peter's point about readability, because
> I agree that "lsn" isn't doing us any favors in that direction.

I disagree that 'lsn' is worse than 'location'- at least for my part,
it's much more precise and clear about what we're talking about.
"location" could be where a file exists on the disk, where in the world
we are, where we are in the heap, where we are when stepping through an
index, etc.  With the work on adding progress information for things
like VACUUM, and I think there was something about tracking progress of
copying a table for logical replication, it certainly seems likely that
we may have cause to think about the point we are in the heap or in an
index.

> So this boils down to whether we are willing to touch any of these
> column names in order to improve consistency.  I think it might be
> worth doing, but there's no doubt that we're adding more compatibility
> pain if we do anything but #1 or #3.

#2 strikes me as the best option, though that's probably not much of a
surprise to anyone whose been following my comments on this thread.

> PS: There are some other changes in David's patch, such as
> s/position/location/ in some text, that I think we should do in any
> case.  But the first decision has to be about the view column names.

Agreed.

Thanks!

Stephen

On 5/9/17 10:00 AM, Stephen Frost wrote:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>
>> #2: Rename all these functions and columns to "lsn", as in this patch.
>
> +1

<...>

> #2 strikes me as the best option, though that's probably not much of a
> surprise to anyone whose been following my comments on this thread.

+1.  If anything this analysis make me more convinced it is the right 
thing to do.

If I read this correctly, we won't change the names of any functions 
that we haven't *already* changed the names of, and only one view would 
be changed to bring it into line with the rest.

-- 
-David
david@pgmasters.net

David Steele <david@pgmasters.net> writes:
> If I read this correctly, we won't change the names of any functions 
> that we haven't *already* changed the names of, and only one view would 
> be changed to bring it into line with the rest.

I have now looked through the patch more carefully, and noted some changes
I forgot to account for in my previous summary: it also renames some
function arguments and output columns, which previously were variously
"location", "wal_position", etc.  I'd missed that for functions that don't
have a formal view in front of them.  This affects

pg_control_checkpoint
pg_control_recovery
pg_create_logical_replication_slot
pg_create_physical_replication_slot
pg_logical_slot_get_binary_changes
pg_logical_slot_get_changes
pg_logical_slot_peek_binary_changes
pg_logical_slot_peek_changes

So that's an additional source of possible compatibility breaks.
It doesn't seem like enough to change anybody's vote on the issue,
but I mention it for completeness.

In terms of the alternatives I listed previously, it seems like
nobody liked alternatives #3, #4, or #5, leaving us with #1 (do
nothing) or #2 (apply this patch).  By my count, Peter is the
only one in favor of doing nothing, and is outvoted.  I'll push
the patch later today if I don't hear additional comments.
        regards, tom lane

On Wed, May 10, 2017 at 1:13 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> David Steele <david@pgmasters.net> writes:
>> If I read this correctly, we won't change the names of any functions
>> that we haven't *already* changed the names of, and only one view would
>> be changed to bring it into line with the rest.
>
> I have now looked through the patch more carefully, and noted some changes
> I forgot to account for in my previous summary: it also renames some
> function arguments and output columns, which previously were variously
> "location", "wal_position", etc.  I'd missed that for functions that don't
> have a formal view in front of them.  This affects
>
> pg_control_checkpoint
> pg_control_recovery
> pg_create_logical_replication_slot
> pg_create_physical_replication_slot
> pg_logical_slot_get_binary_changes
> pg_logical_slot_get_changes
> pg_logical_slot_peek_binary_changes
> pg_logical_slot_peek_changes
>
> So that's an additional source of possible compatibility breaks.
> It doesn't seem like enough to change anybody's vote on the issue,
> but I mention it for completeness.
>
> In terms of the alternatives I listed previously, it seems like
> nobody liked alternatives #3, #4, or #5, leaving us with #1 (do
> nothing) or #2 (apply this patch).  By my count, Peter is the
> only one in favor of doing nothing, and is outvoted.  I'll push
> the patch later today if I don't hear additional comments.

For the record, I also voted for doing nothing.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, May 10, 2017 at 1:13 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> In terms of the alternatives I listed previously, it seems like
>> nobody liked alternatives #3, #4, or #5, leaving us with #1 (do
>> nothing) or #2 (apply this patch).  By my count, Peter is the
>> only one in favor of doing nothing, and is outvoted.  I'll push
>> the patch later today if I don't hear additional comments.

> For the record, I also voted for doing nothing.

Hm, well, anybody else want to vote?
        regards, tom lane

On 05/10/2017 12:22 PM, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Wed, May 10, 2017 at 1:13 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> In terms of the alternatives I listed previously, it seems like
>>> nobody liked alternatives #3, #4, or #5, leaving us with #1 (do
>>> nothing) or #2 (apply this patch).  By my count, Peter is the
>>> only one in favor of doing nothing, and is outvoted.  I'll push
>>> the patch later today if I don't hear additional comments.
>
>> For the record, I also voted for doing nothing.
>
> Hm, well, anybody else want to vote?

+1 for #2

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

On 10/05/17 21:22, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Wed, May 10, 2017 at 1:13 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> In terms of the alternatives I listed previously, it seems like
>>> nobody liked alternatives #3, #4, or #5, leaving us with #1 (do
>>> nothing) or #2 (apply this patch).  By my count, Peter is the
>>> only one in favor of doing nothing, and is outvoted.  I'll push
>>> the patch later today if I don't hear additional comments.
> 
>> For the record, I also voted for doing nothing.
> 
> Hm, well, anybody else want to vote?
> 

I am for standardizing (although I don't have preference of location vs
lsn).

--  Petr Jelinek                  http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training &
Services

On Wed, May 10, 2017 at 01:09:36PM -0700, Joe Conway wrote:
> On 05/10/2017 12:22 PM, Tom Lane wrote:
> > Robert Haas <robertmhaas@gmail.com> writes:
> >> On Wed, May 10, 2017 at 1:13 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >>> In terms of the alternatives I listed previously, it seems like
> >>> nobody liked alternatives #3, #4, or #5, leaving us with #1 (do
> >>> nothing) or #2 (apply this patch).  By my count, Peter is the
> >>> only one in favor of doing nothing, and is outvoted.  I'll push
> >>> the patch later today if I don't hear additional comments.
> > 
> >> For the record, I also voted for doing nothing.
> > 
> > Hm, well, anybody else want to vote?
> 
> +1 for #2

Same, +1 for #2 (apply this patch)

--  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 +

On Thu, May 11, 2017 at 9:15 AM, Bruce Momjian <bruce@momjian.us> wrote:
> On Wed, May 10, 2017 at 01:09:36PM -0700, Joe Conway wrote:
>> On 05/10/2017 12:22 PM, Tom Lane wrote:
>> > Robert Haas <robertmhaas@gmail.com> writes:
>> >> On Wed, May 10, 2017 at 1:13 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> >>> In terms of the alternatives I listed previously, it seems like
>> >>> nobody liked alternatives #3, #4, or #5, leaving us with #1 (do
>> >>> nothing) or #2 (apply this patch).  By my count, Peter is the
>> >>> only one in favor of doing nothing, and is outvoted.  I'll push
>> >>> the patch later today if I don't hear additional comments.
>> >
>> >> For the record, I also voted for doing nothing.
>> >
>> > Hm, well, anybody else want to vote?
>>
>> +1 for #2
>
> Same, +1 for #2 (apply this patch)

#1, do nothing.
-- 
Michael

Michael Paquier <michael.paquier@gmail.com> writes:
> On Thu, May 11, 2017 at 9:15 AM, Bruce Momjian <bruce@momjian.us> wrote:
>> On Wed, May 10, 2017 at 01:09:36PM -0700, Joe Conway wrote:
> On 05/10/2017 12:22 PM, Tom Lane wrote:
>>> Hm, well, anybody else want to vote?

>>> +1 for #2

>> Same, +1 for #2 (apply this patch)

> #1, do nothing.

OK, by my count we have

For patch:
Joe Conway
Stephen Frost
Kyotaro Horiguchi (*)
Petr Jelinek (*)
Tom Lane
Bruce Momjian
David Rowley
David Steele
Euler Taveira

For doing nothing:
Peter Eisentraut
Robert Haas
Michael Paquier

I think Kyotaro-san and Petr would have preferred standardizing
on "location" over "lsn", but they were definitely not for doing
nothing.  With or without their votes, it's pretty clear that the
ayes have it.
        regards, tom lane

Neha Khatri <nehakhatri5@gmail.com> writes:
> [In case forgotten] pg_controdata and pg_waldump interfaces should also be
> considered for this standardization.

> Following are pg_controldata interfaces that might require change:

>   Latest checkpoint location:
>   Prior checkpoint location:
>   Latest checkpoint's REDO location:
>   Minimum recovery ending location:
>   Backup start location:
>   Backup end location:

My inclination is to leave these messages alone.  They're not really
inconsistent with anything.  Where we seem to be ending up is that
"lsn" will be used in things like function and column names, but
documentation will continue to spell out phrases like "WAL location".

There is another open thread about converting said phrases to be
more consistent --- a lot of them currently say "transaction log
location", which is not a very good choice because it invites
confusion with pg_xact nee pg_clog.  But I think that's mostly
just documentation changes, and in any case it's better done as
a separate patch.

> The pg_waldump help options(and probably documentation) would also require
> change:
>   -e, --end=RECPTR       stop reading at log position RECPTR
>   -s, --start=RECPTR     start reading at log position RECPTR

Yeah, probably s/log position/WAL location/ would be better here.
But again that seems like material for a separate patch.  The
current patch does do s/position/location/ in a few places, but
it's not trying to apply that policy everywhere.
        regards, tom lane

Neha Khatri <nehakhatri5@gmail.com> writes:
> On Fri, May 12, 2017 at 12:56 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Neha Khatri <nehakhatri5@gmail.com> writes:
>>> Following are pg_controldata interfaces that might require change:
>>> Latest checkpoint location:
>>> Prior checkpoint location:
>>> Latest checkpoint's REDO location:
>>> Minimum recovery ending location:
>>> Backup start location:
>>> Backup end location:

>> My inclination is to leave these messages alone.  They're not really
>> inconsistent with anything.  Where we seem to be ending up is that
>> "lsn" will be used in things like function and column names, but
>> documentation will continue to spell out phrases like "WAL location".

> Are you indicating that the above phrases do not require change because
> those are consistent with other references. Or the other thread [1]
> (renaming 'transaction log') should take care of it.

Personally I'm happy to leave those particular messages as they are.
Yes, a case could be made for changing them to say "LSN", and a different
case could be made for changing them to say "WAL location", but I don't
think either of those are real improvements.  Also, this'd be propagating
the compatibility problem into yet a new place, because there are surely
user-written scripts out there that grep the output for exactly these
spellings.

It's a judgment call though, and others might have different opinions.
        regards, tom lane