Should pg_current_wal_location() become pg_current_wal_lsn()

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_replication
write_location | pg_stat_replication
flush_location | pg_stat_replication
replay_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 | normal
pg_catalog | pg_lsn_eq | boolean | pg_lsn, pg_lsn | normal
...
pg_catalog | pg_lsn_recv | pg_lsn | internal | normal
pg_catalog | pg_lsn_send | bytea | pg_lsn | normal
(13 rows)

=# 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..

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

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

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

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

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

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

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 +

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

* 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

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

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

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

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

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

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

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

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.srsublsn
pg_stat_replication.sent_location
pg_stat_replication.write_location
pg_stat_replication.flush_location
pg_stat_replication.replay_location
pg_stat_wal_receiver.receive_start_lsn
pg_stat_wal_receiver.received_lsn
pg_stat_wal_receiver.latest_end_lsn
pg_stat_subscription.received_lsn
pg_stat_subscription.latest_end_lsn
pg_replication_slots.restart_lsn
pg_replication_slots.confirmed_flush_lsn
pg_replication_origin_status.remote_lsn
pg_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 irrelevant
pg_current_wal_location
pg_current_wal_insert_location
pg_current_wal_flush_location
pg_wal_location_diff
pg_last_wal_receive_location
pg_last_wal_replay_location
pg_lsn_mi
pg_lsn_in
pg_lsn_out
pg_lsn_lt
pg_lsn_le
pg_lsn_eq
pg_lsn_ge
pg_lsn_gt
pg_lsn_ne
pg_lsn_recv
pg_lsn_send
pg_lsn_cmp
pg_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

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

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.

/messages/by-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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

+1.

--
Euler Taveira Timbira -
http://www.timbira.com.br/
PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento
<http://www.timbira.com.br&gt;

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